DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.11k stars 256 forks source link

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

Closed mmuetzel closed 1 month ago

mmuetzel commented 3 months ago

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.

DrTimothyAldenDavis commented 3 months ago

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 commented 3 months ago

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