[POC] cmake: Add option to run include-what-you-use with compiler #1204

1. 
   hebasto commented at 1:50 pm on January 31, 2023: member

   IWYU could be useful for developers, e.g., #924 etc.

   Example of usage on Ubuntu 22.04:

   0$ cmake -S . -B build -DSECP256K1_ENABLE_IWYU=ON -DCMAKE_C_COMPILER=clang
   1$ cmake --build build
   

2. 
   real-or-random commented at 2:04 pm on January 31, 2023: contributor

   Oh, this may indeed be helpful. I haven’t tested anything yet, but if this helps towards #1039, I’m interested.
3. cmake: Add option to run `include-what-you-use` with compiler 5a91a4c536
4. hebasto force-pushed on Mar 8, 2023
5. hebasto marked this as ready for review on Mar 8, 2023
6. 
   hebasto commented at 4:40 pm on March 8, 2023: member

   Rebased on top of the #1113.
7. 
   real-or-random commented at 9:13 am on March 9, 2023: contributor

   I fear include-what-you-use is too dump for our file organization and won’t help towards making files self-contained #924.

   For example, it suggests:

   0The full include-list for /home/tim/bs/dev/secp256k1/src/secp256k1.c:
   1(...)
   2#include "field_5x52_impl.h"                    // for secp256k1_fe_get_b32
   3(...)
   

   We don’t want to include field_5x52_impl.h because we include field_impl.h (which handles selection of the right full file)

   Also, it suggests includes only for .c files and not for .h files… If we want to have self-contained files, it may be much better to simply check that every file complies individually as suggested in #924.

8. 
   hebasto commented at 10:48 am on March 9, 2023: member

   Also, it suggests includes only for .c files and not for .h files…

   For example:

   0/home/hebasto/git/secp256k1/src/precomputed_ecmult.h should add these lines:
   1#include "ecmult.h"  // for ECMULT_TABLE_SIZE
   

9. 
   hebasto commented at 3:43 pm on March 9, 2023: member

   I fear include-what-you-use is too dump for our file organization…

   Sure, it is not a panacea. However, it can be useful. For instance, consider the demo branch (a mix with #1231):

   0$ cmake -S . -B build -DSECP256K1_ENABLE_IWYU=ON -DCMAKE_C_COMPILER=clang
   1$ cmake --build build --target precomputed  # no IWYU diagnostic
   

10. 
    hebasto commented at 2:05 pm on March 10, 2023: member

    @real-or-random

    We don’t want to include field_5x52_impl.h because we include field_impl.h (which handles selection of the right full file)

    True. And it can be achieved and nicely self documented as follows:

    0/* IWYU pragma: begin_exports */
    1#if defined(SECP256K1_WIDEMUL_INT128)
    2#include "field_5x52_impl.h"
    3#elif defined(SECP256K1_WIDEMUL_INT64)
    4#include "field_10x26_impl.h"
    5#else
    6#error "Please select wide multiplication implementation"
    7#endif
    8/* IWYU pragma: end_exports */
    

    FWIW, another demo branch has no IWYU diagnostics for secp256k1 and secp256k1_static targets.

11. 
    fanquake commented at 2:18 pm on March 10, 2023: member

    Concept ~0. Cores experience with IWYU has been mixed. We’ve found plenty of bugs/nonsensical include suggestions, and it’s not ideal if by default you have to add comments all over the code. We should probably look at migrating to something else, maybe LLVMs clang-include-fixer tool.

    Generally I’m not sure that “convenience option for running an include fixer” is something that needs to exists in libsecp256k1s build system. Running include-what-you-use yourself is pretty straight forward.

12. 
    hebasto commented at 5:39 pm on April 28, 2023: member

    Running include-what-you-use yourself is pretty straight forward.

    I agree with that. For example,

    0cmake -S . -B build -DCMAKE_C_COMPILER=clang -DCMAKE_C_INCLUDE_WHAT_YOU_USE=$(command -v include-what-you-use)
    1cmake --build build
    

    Closing.

13. hebasto closed this on Apr 28, 2023

    ---

14. hebasto deleted the branch on Jun 6, 2023

Contributors
hebasto real-or-random fanquake