json icon indicating copy to clipboard operation
json copied to clipboard

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

Open laramiel opened this issue 1 year ago • 7 comments

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.

laramiel avatar Oct 16 '24 00:10 laramiel

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>.

gregmarr avatar Oct 16 '24 01:10 gregmarr

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

nlohmann avatar Oct 16 '24 05:10 nlohmann

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?

laramiel avatar Oct 16 '24 16:10 laramiel

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.

gregmarr avatar Oct 16 '24 16:10 gregmarr

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

laramiel avatar Oct 16 '24 16:10 laramiel

Can you please rebase to the develop branch? I'll check this then.

nlohmann avatar Nov 20 '24 12:11 nlohmann

This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions!

github-actions[bot] avatar Jan 18 '25 00:01 github-actions[bot]