Make files mostly self-contained #1724
Conversation
real-or-random
added
tweak/refactor
meta/development
real-or-random
force-pushed
the
202508-selfcontained
branch
from
61f1562 to
925070e
Compare
Remaining pain points that I noticed
|
There was a problem hiding this comment.
I think what I picked is fine but I'm happy to take suggestions for the name of this file.
| #include "modinv32_impl.h" | ||
| #ifdef SECP256K1_WIDEMUL_INT128 | ||
| /* modinv64 is only available if we have an int128 implementation. */ | ||
| #include "modinv64_impl.h" | ||
| #endif |
There was a problem hiding this comment.
That part is a bit ugly, in particular because it needs to be duplicated in multiple C files.
Alternatives that I didn't find better:
- Make including
modinv64_impl.ha no-op ifndefSECP256K1_WIDEMUL_INT128. If anything, that violates the principle of least surprise. - Create a
modinv_impl.hthat selects between the two files. That's possible, but conceptually not really the right thing to do.modinv64andmodinv32are two separate modules with different interface. They're not just two different implementations of the same interfaces (as is the case for the different field, scalar, int128 implementations).
real-or-random
force-pushed
the
202508-selfcontained
branch
2 times, most recently
from
5eebcd0 to
024e154
Compare
This has the advantage of keeping the ecmult tests focused on the ecmult code, and so this change might be desirable anyway. That's why it is done in a separate commit, so it can be kept if we ever bring back the public scratch space API.
These were in the public API at some point but they have been made static in 712e7f8.
We don't use precompiled headers in our builds but those files are what you get if you invoke gcc or clang, respectively, on a .h file, e.g., to check if it's self-contained.
Otherwise, the repr headers field_AxB.h would need to include field.h to be self-contained but this recursive inclusion leaves clangd confused, see clangd/clangd#337 .
secp256k1.c is included anyway, so there's no need to include the individual _impl.h files.
SECP256K1_B and secp256k1_ge_const_g are used outside of group_impl.h, e.g., in ecmult_const_impl.h and ecmult_gen_impl.h, respectively.
And also sort the _impl.h includes.
real-or-random
force-pushed
the
202508-selfcontained
branch
from
7e2f395 to
8cce392
Compare
|
Added another commit that fixes a missing declaration in |
Include what you use (as far as possible). This is currently a bit arbitrary in the entire library, but see bitcoin-core#1724 for rationale why this is a good idea.
FWIW, #1423 started with the introduction of such a tool. I still think this should be required in any PR that attempts to improve header inclusion. |
I've rebased #1423 to check that the code is still valid. Feel free to incorporate it here if you think it's worth it. UPD. Here is a combined branch that applies the test to as many headers as possible. |
| static int secp256k1_selftest_passes(void) { | ||
| return secp256k1_selftest_sha256(); | ||
| } | ||
| static int secp256k1_selftest_sha256(void); |
There was a problem hiding this comment.
Shouldn't this function be internal to selftest?
| #include "selftest.h" | ||
| #include "util.h" | ||
|
|
||
| #include <string.h> |
| #include "hash.h" | ||
| #include "selftest.h" | ||
| #include "util.h" |
There was a problem hiding this comment.
I have a general suggestion. Since every <name>_impl.h must include <name>.h, I think readability would improve if <name>.h always appeared first on its own line, as shown here:
| #include "hash.h" | |
| #include "selftest.h" | |
| #include "util.h" | |
| #include "selftest.h" | |
| #include "hash.h" | |
| #include "util.h" |
While these rules indeed optimize performance regardless of LTO, they do a poor job of hiding implementation details for internal “modules” consisting of a header ( /* foo.h */
#ifndef UNITY_BUILD_DEMO_FOO_H
#define UNITY_BUILD_DEMO_FOO_H
static int foo();
#endif /* UNITY_BUILD_DEMO_FOO_H *//* foo_impl.h */
#ifndef UNITY_BUILD_DEMO_FOO_IMPL_H
#define UNITY_BUILD_DEMO_FOO_IMPL_H
#include "foo.h"
#include "foobar.h"
static int foo() { return foobar(); }
#endif /* UNITY_BUILD_DEMO_FOO_IMPL_H *//* foobar.h */
#ifndef UNITY_BUILD_DEMO_FOOBAR_H
#define UNITY_BUILD_DEMO_FOOBAR_H
static int foobar();
#endif /* UNITY_BUILD_DEMO_FOOBAR_H *//* foobar_impl.h */
#ifndef UNITY_BUILD_DEMO_FOOBAR_IMPL_H
#define UNITY_BUILD_DEMO_FOOBAR_IMPL_H
#include "foobar.h"
static int foobar() { return 42; }
#endif /* UNITY_BUILD_DEMO_FOOBAR_IMPL_H *//* main.c */
#include "foo_impl.h"
#include "foobar_impl.h"
#include <stdlib.h>
int main(void) {
int a = foo();
return EXIT_SUCCESS;
}In this example, the call to A proper additional rule, I believe, would be to allow implementation files to include other implementation files where needed. This yields the following structure: /* foo_impl.h */
#ifndef UNITY_BUILD_DEMO_FOO_IMPL_H
#define UNITY_BUILD_DEMO_FOO_IMPL_H
#include "foo.h"
#include "foobar_impl.h"
static int foo() { return foobar(); }
#endif /* UNITY_BUILD_DEMO_FOO_IMPL_H *//* main.c */
#include "foo_impl.h"
#include <stdlib.h>
int main(void) {
int a = foo();
return EXIT_SUCCESS;
}I have analyzed this approach for potential drawbacks and have not found any. |
|
Could you please elaborate on your motivation for 391d16a "Create header for internal API in secp256k1.c"? Why not move the implementation code from UPD. I've completed my first round of reviewing. The code looks correct. |
This is a set of commits, each one trivial, that reorganizes code so that all C files are mostly self-contained.
These changes are merely fixes that make the files adhere to our current implicit style, which I'd describe as:
.h) and implementation files (_impl.h). Symbols not present in the header are private to that module.secp256k1.cincludes all_impl.hfiles. Other C files include simplysecp256k1.c, or, if that isn't possible for some reason (e.g., in theprecompute_*.cfiles), the individual necessary_impl.hfiles.So, none of these changes here should be controversial. In particular, I refrain from changing the above rules (e.g., by renaming the
_impl.hto.cas suggested in #1039).Why "mostly" self-contained?
After this PR, some errors remain when I run
clang src/*.candclang src/*.hon my machine. But all of them are special cases:ctime_tests.crefuses to be compiled without valgrind or msan enabled.src/field_10x26_impl.h,src/int128_struct_impl.h, andsrc/scalar_low_impl.hare confused because the autodetection logic in the preprocessor picks field_5x64, native int 128, and scalar_4x64 on my machine.Full output of "clang src/*.c" and "clang src/*.h`
Perhaps some of these cases can be improved further, but these improvements should go to a separate PR.
What is the motivation for these changes?
Self-contained files work much better with tooling. For example, the
[clangd](https://clangd.llvm.org/installation)language server works great in my editor after this PR (ignoring the four files mentioned above) and gives me much better completion, symbol lookup, etc. In fact, what made me create this PR is that I got annoyed by not being able to jump to symbols when trying to review silent payments.Here's my
~/.config/clangd/config.yamlif you're interested:You'll also need to run
make cleanandbear -- makeonce to create acompile_commands.jsonfile in the repo root that contains the compiler flags so that clangd knows the configured-Dflags etc. Get bear first.edit: CMake can also generate the json file:
Then the file will be stored in the
buildsubdirectory, whose name is special-cased in clangd so that it the file will be found automatically.Or the flags can be added above in
CompileFlagsmanually.