-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
Conversation
fixes sourcemeta#1684 Signed-off-by: Balakrishna Avulapati <bavulapati@gmail.com>
@@ -1,10 +1,20 @@ | |||
#include <sourcemeta/core/json_value.h> |
There was a problem hiding this comment.
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
#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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <sourcemeta/core/json_value.h> | |
#include <sourcemeta/core/json.h> |
@@ -1,10 +1,19 @@ | |||
#include <sourcemeta/core/json_value.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <sourcemeta/core/json_value.h> | |
#include <sourcemeta/core/json.h> |
@@ -1,7 +1,17 @@ | |||
#include <sourcemeta/core/json_value.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <sourcemeta/core/json_value.h> | |
#include <sourcemeta/core/json.h> |
#include <sourcemeta/core/json_error.h> | ||
#include <sourcemeta/core/json_value.h> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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
@jviotti I think we can use https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md I'm learning more |
I think we are looking for https://clangd.llvm.org/design/include-cleaner#umbrella-headers |
Not sure how googletest is using it. repo doesn't have any file containing |
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 |
fixes #1684