Skip to content

style(clang-tidy): fix include headers clang-tidy errors #1688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bavulapati
Copy link
Contributor

fixes #1684

fixes sourcemeta#1684

Signed-off-by: Balakrishna Avulapati <bavulapati@gmail.com>
@@ -1,10 +1,20 @@
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make modules only include public headers of other modules

Suggested change
#include <sourcemeta/core/json_value.h>
#include <sourcemeta/core/json.h>

The idea is that only src/json can be internally aware that there is a json_value.h header, and every consumer should just require the public one

@@ -1,11 +1,21 @@
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <sourcemeta/core/json_value.h>
#include <sourcemeta/core/json.h>

@@ -1,5 +1,10 @@
#include <sourcemeta/core/json.h>
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't include json_value.h if we are including json.h

@@ -1,4 +1,8 @@
#include <sourcemeta/core/jsonschema.h>
#include <sourcemeta/core/jsonschema_types.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't include jsonschema_types.h if we are including jsonschema.h

@@ -1,9 +1,21 @@
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <sourcemeta/core/json_value.h>
#include <sourcemeta/core/json.h>

@@ -1,10 +1,19 @@
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <sourcemeta/core/json_value.h>
#include <sourcemeta/core/json.h>

@@ -1,7 +1,17 @@
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <sourcemeta/core/json_value.h>
#include <sourcemeta/core/json.h>

#include <sourcemeta/core/json_error.h>
#include <sourcemeta/core/json_value.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't include json_value.h (nor json_error.h as in the above line btw) if we are including json.h

#include <sourcemeta/core/yaml.h>
#include <sourcemeta/core/yaml_error.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't include yaml_error.h if we are including yaml.h

@@ -1,11 +1,14 @@
#include <sourcemeta/core/alterschema.h>

#include <cassert> // assert
#include <sourcemeta/core/jsonschema_transform.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should only be including jsonschema.h

@bavulapati
Copy link
Contributor Author

@bavulapati
Copy link
Contributor Author

@bavulapati
Copy link
Contributor Author

I think we are looking for https://clangd.llvm.org/design/include-cleaner#umbrella-headers

@bavulapati
Copy link
Contributor Author

Not sure how googletest is using it. repo doesn't have any file containing iwyu other than source code

@jviotti
Copy link
Member

jviotti commented May 25, 2025

I'm not sure, but sounds worth researching. In any case, just to keep it simple, let's aim to fix the ClangTidy warnings we get now, as of the latest changes. Once we have ClangTidy enforced in the .cc files, we can look at how to make it cover headers and potentially improve the include detection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClangTidy warnings for misc-include-cleaner
2 participants