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

CI (ubuntu): Use "include-what-you-use" in one configuration. #792

Closed
wants to merge 1 commit into from

Conversation

mmuetzel
Copy link
Contributor

Building with "include-what-you-use" increases the build time. Since we don't need to scan every compilation unit twice, add this on top of the runner that only builds static libraries.

The (initial) results can't be perfect because the tool tries to infer the intent of the author. And that might vary in principle. (See https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhatIsAUse.md)
However, the results could be improved by adding iwyu pragmas, e.g., // IWYU pragma: keep after headers you'd like to keep. (See https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md)
After some initial "tagging", the results could probably help to detect compilation units where some STL headers "just happen" to be included currently. Having this check might have helped to avoid issues like #769.

I could try to add those iwyu pragmas or add missing headers with the results of this check. But I'll wait for you to decide if you think this is worth it (it is imho) or if you think those additional comments (pragmas) are clutter that you prefer to not have in your sources.

Building with "include-what-you-use" slightly increases the build time.
Since we don't need to scan every compilation unit twice, add this on top
of the runner that only builds static libraries.

Having this check might have helped to avoid issues like DrTimothyAldenDavis#769.
@DrTimothyAldenDavis
Copy link
Owner

I don't understand what "include what you use" means. So any pragmas added to the source code would be a mystery to me. I don't want to add any mystery code to the source code.

@mmuetzel
Copy link
Contributor Author

include-what-you-use is a tool that checks if compilation units (directly) include headers for the symbols that they use.
E.g., for one of the files from the AMD library, it reported:

Warning: include-what-you-use reported diagnostics:
  
  /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_1.c should add these lines:
  #include <stdint.h>        // for int32_t
  
  /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_1.c should remove these lines:
  
  The full include-list for /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_1.c:
  #include <stdint.h>        // for int32_t
  #include "amd_internal.h"  // for ASSERT, Int, AMD_1, AMD_2, AMD_DEBUG1
  ---

That probably means that int32_t happens to be defined because some independent header contains a #include <stdint.h>. But a re-organization (of the standard library headers?) might lead to that symbol being no longer defined in that compilation unit. So, it would probably make sense to follow the suggestion and explicitly include that header in that file. (Or maybe better in amd_internal.h or in amd.h.)

On the other hand, it reports for another file:

 Warning: include-what-you-use reported diagnostics:
  
  /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_dump.c should add these lines:
  
  /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_dump.c should remove these lines:
  - #include "amd_internal.h"  // lines 16-16
  
  The full include-list for /home/runner/work/SuiteSparse/SuiteSparse/AMD/Source/amd_dump.c:

But I'd guess that you might prefer to include that header in all files (even if it is not strictly necessary).
To silence that warning, a comment could be added behind that inclusion directive like so:

#include "amd_internal.h" // IWYU pragma: keep

@mmuetzel mmuetzel closed this May 30, 2024
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.

2 participants