Refactor includes

[whb07]

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

1. Headers defined by the secp256k1 library are now imported via #include "secp256k1.h".
2. 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.

[sipa]

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").

[whb07]

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?

[real-or-random]

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

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

#include <string.h>
#include <secp256k1.h>

#include "lax_der_parsing.h"

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

#include "include/secp256k1.h"
...

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?

[real-or-random]

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,

make -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]

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