Skip to content

Conversation

@bavulapati
Copy link
Contributor

fixes #1684

@@ -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.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/json_error.h>
#include <sourcemeta/core/json_value.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

#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?

@bavulapati
Copy link
Contributor Author

#1750 replaces this.

@bavulapati bavulapati closed this Jun 22, 2025
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