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

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

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 31, 2023

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

Example of usage on Ubuntu 22.04:

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

@real-or-random
Copy link
Contributor

Oh, this may indeed be helpful. I haven't tested anything yet, but if this helps towards #1039, I'm interested.

@hebasto hebasto marked this pull request as ready for review March 8, 2023 16:39
@hebasto
Copy link
Member Author

hebasto commented Mar 8, 2023

Rebased on top of the #1113.

@real-or-random
Copy link
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:

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

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.

@hebasto
Copy link
Member Author

hebasto commented Mar 9, 2023

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

For example:

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

@hebasto
Copy link
Member Author

hebasto commented Mar 9, 2023

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):

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

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023

@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:

/* IWYU pragma: begin_exports */
#if defined(SECP256K1_WIDEMUL_INT128)
#include "field_5x52_impl.h"
#elif defined(SECP256K1_WIDEMUL_INT64)
#include "field_10x26_impl.h"
#else
#error "Please select wide multiplication implementation"
#endif
/* IWYU pragma: end_exports */

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

@fanquake
Copy link
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.

@hebasto
Copy link
Member Author

hebasto commented Apr 28, 2023

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

I agree with that. For example,

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

Closing.

@hebasto hebasto closed this Apr 28, 2023
@hebasto hebasto deleted the 230131-iwyu branch June 6, 2023 08:16
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.

3 participants