Skip to content
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

Add missing #includes based on C++ language server. #4477

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

laramiel
Copy link

Add missing #includes based on IWYU service.

Mostly this adds #include <nlohmann/detail/abi_macros.hpp> since NLOHMANN_JSON_NAMESPACE_BEGIN appears
nearly everywhere.

It adds a few other includes as well.

@gregmarr
Copy link
Contributor

Honestly I don't see the benefit of including nlohmann/detail/abi_macros.hpp and nlohmann/thirdparty/hedley/hedley.hpp in files that are also including nlohmann/detail/macro_scope.hpp.

https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

Every .h file you bring in when compiling a source file lengthens the time to compile, as the bytes have to be read, preprocessed, and parsed. If you're not actually using a .h file, you remove that cost. With template code, where entire instantiations have to be in .h files, this can be hundreds of thousands of bytes of code. In one case at Google, running include-what-you-use over a .cc file improved its compile time by 30%.

Here, the main benefit of include-what-you-use comes from the flip side: "don't include what you don't use."

The thing this is mostly doing is increasing the compile time by adding extra includes of header files that are already included.

refactor could break code far away from you.

This is most compelling for a very large codebase (such as Google's). In a small codebase, it's practical to just compile everything after a refactor like this, and clean up any errors you see.

I would say that this is a small codebase, and not only is it practical to just compile it, it is basically required by the amalgamation.

Removing <iostream> and <sstream> from the tests is actually helpful, as it reduces the compile time, but there is then no reason to add <cstdint>, <vector>, <cassert>, <cstddef>.

@nlohmann
Copy link
Owner

I am also hesitant: it seems to fix a problem we don't have.

@laramiel
Copy link
Author

These includes follow the "Include-what-you-spell" principle; c++ language servers highlight these are missing.

RE macro_scope. Shouldn't each #include of that file be matched by an include of macro_unscope?

@gregmarr
Copy link
Contributor

These includes follow the "Include-what-you-spell" principle; c++ language servers highlight these are missing.

This change violates the IWYU Faster compiles principle, and for many users of this header-only library, that's a more important principle than what opinion c++ language servers have about the details of the internal headers. If you use the single include version at https://github.com/nlohmann/json/blob/develop/single_include/nlohmann/json.hpp instead of the multiple include version, then your c++ language server won't even notice.

RE macro_scope. Shouldn't each #include of that file be matched by an include of macro_unscope?

No, it's included once at https://github.com/nlohmann/json/blob/develop/include/nlohmann/json.hpp#L5256. The headers in the detail directory are not designed to be included directly by end users.

@laramiel
Copy link
Author

laramiel commented Oct 16, 2024

I have removed most of the #includes changes, audited the use of macro_scope vs abi_macros to only include one or the other, and to avoid all includes of hedley outside of macro scope.

I added an IWYU pragma comment to macro_scope to make the language server happy.

The other include changes are for types used in files which have no exports.

PTAL

https://clangd.llvm.org/design/include-cleaner

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

Successfully merging this pull request may close these issues.

3 participants