Refactor includes

whb07 commented at 7:55 pm on April 24, 2021: contributor

Two primary changes based on the way include statements are handled for the project.

Headers defined by the secp256k1 library are now imported via #include "secp256k1.h".
Changes to the files that got included are based on the “include what you use” tool.

For the first point to happen, the Makefile.am got tweaked and declared the AM_CPPFLAGS variable and included as a dependency for the different targets on the file. I do not have extensive autoconf/automake experience, so I just went with this variable and while it “works”, I am not clear if it is the best way to do it.

At any rate, within say secp256k1.c, one can include the header in the cleaner syntax without the “include/” prefix. This also allows other external libraries and build configs to work by defining the lib’s include path as such.

The latter point makes use of the aggregate tooling built on Clang’s different tools called “include what you use”. Essentially it shows what needs to be declared/removed to include what you use.

changed include/*.h to "*.h" for headers cea30df3a6

ran include-what-you-use" and fixed based on those results. Added flag in Makefile.am for standardizing of secp256k1.h statements b027188769

added missing valgrind/valgrind.h include and removed assumptions.h unused header 70dd316df4

removed include of libsecp256-config header 79c01aa9a8

moved order of imports c228fde405

moved order of imports back to somewhat original bd8da4096a

moved order of imports back to somewhat original 94372629aa

moved order of imports back to somewhat original a506177e9b

moved order of imports back to somewhat original f2df8ad368

moved order of imports back to somewhat original a2ab1ee6e8

moved order of imports back to somewhat original e21b43436c

added back assumptions.h inclusion 4bcf339377

sipa commented at 8:07 pm on April 24, 2021: contributor

At least the implementation choice-specific includes are incorrect (e.g. you should include “field.h” instead of “field_5x52.h”, and “scalar.h” instead of “scalar_4x64.h”).

cleaned up the includes, mostly back to the original state fae3e21e17

removed whitespace 3a6a1a2877

whb07 commented at 8:23 pm on April 24, 2021: contributor

At least the implementation choice-specific includes are incorrect (e.g. you should include “field.h” instead of “field_5x52.h”, and “scalar.h” instead of “scalar_4x64.h”).

Yes I am starting to see that! Well partially it appears, a tricky devil this project uses some indirection based on implementations but that then get included via that indirection. I am going to be manually editing those for proper declarations.

Outside of that @sipa what do you make of the Makefile.am change? Like i mentioned before I am no expert in that tooling so I have no strong opinions and need proper guidance. One thing to note is that the “SECP_INCLUDES” variable appears always empty, is that on purpose, or some dead code? Or does that ever have a value in other build systems?

removed scalar_* specific and added scalar_impl.h 0afe52cc50

restored back basic 'header_impl' includes' 9c810e8de9

mostly restored include statements as they were if they were of the '_impl.h' type b9b02ae96b

real-or-random commented at 3:05 pm on April 25, 2021: contributor

@whb07

Is the purpose of this PR simply to make the code cleaner or is this solving any particular issue that you have when using the library?

whb07 commented at 5:03 pm on April 25, 2021: contributor

@whb07

Is the purpose of this PR simply to make the code cleaner or is this solving any particular issue that you have when using the library?

Both! Which is leading me to believe that it might be best to split the two main changes into individual PRs. Building separately from source using a different build system has the issues of the different lib’s inclusion.

The source files in contrib refer to the header here as :

0#include <string.h>
1#include <secp256k1.h>
2
3#include "lax_der_parsing.h"

Which is fine and dandy but in the files within src/ they are referred as:

0
1#include "include/secp256k1.h"
2...

Then when building out of source, you end up with the error of “include/secp256k1.h” not found. So this cleans up the syntax and styling, whilst at the same time clearing up the differing build issues.

What do you think?

whb07 cross-referenced this on Apr 25, 2021 from issue changed include statements without prefix 'include/' by whb07

real-or-random referenced this in commit 185a6af227 on May 5, 2021

real-or-random commented at 1:37 pm on May 18, 2021: contributor

So now that #925 has been merged, the point of this PR would be to tidy up the remaining includes.

That’s not strictly necessary but it’s a good idea from time to time. @whb07 Do you want to tidy and squash the changes here?

Unfortunately we cannot fully automate the include stuff at this point, due to our special implementation-specific includes. Otherwise we could add a check like to the CI. For example,

0make -k CC='include-what-you-use -Xiwyu --keep=src/assumptions.h -Xiwyu --no_comments

comes pretty close to what we need. Maybe it’s possible to make it work with the --mapping_file argument, not sure.

the Makefile.am got tweaked and declared the AM_CPPFLAGS variable and included as a dependency for the different targets on the file. I do not have extensive autoconf/automake experience, so I just went with this variable and while it “works”, I am not clear if it is the best way to do it.

I think this change should be removed from the PR then, since the point of #925 was exactly the opposite, namely to avoid more -I arguments.

real-or-random cross-referenced this on Dec 10, 2021 from issue Make all source and header files self-contained by real-or-random

hebasto cross-referenced this on Jan 31, 2023 from issue [POC] cmake: Add option to run `include-what-you-use` with compiler by hebasto

hebasto cross-referenced this on Mar 9, 2023 from issue Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` by hebasto

real-or-random referenced this in commit a6f4bcf6e1 on Apr 20, 2023

real-or-random commented at 4:16 pm on May 8, 2023: contributor

Closing this PR for now due to lack of activity, it can always be reopenend. (This is somewhat tracked in #1039 in a sense that any cleaning of includes should ideally solve #1039 or make progress towards it.)

real-or-random closed this on May 8, 2023

Refactor includes #924