While reviewing #1734 and suggesting keeping the new test miniframework as a separate translation unit, I noticed a couple of issues with the headers included in testrand_impl.h. In my approach, these lead to compiler warnings and linker errors. This PR fixes them.
These changes are reasonable on their own.
9 | @@ -10,9 +10,10 @@ 10 | #include <stdint.h> 11 | #include <stdio.h> 12 | #include <string.h> 13 | +#include <time.h>
That one is also in #1724. (Want to review it? :) )
9 | @@ -10,9 +10,10 @@ 10 | #include <stdint.h> 11 | #include <stdio.h> 12 | #include <string.h> 13 | +#include <time.h> 14 | 15 | #include "testrand.h" 16 | -#include "hash.h" 17 | +#include "hash_impl.h"
Why do we need this?
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails:
$ cmake --build build
[1/20] Building C object src/CMakeFiles/unit_test.dir/unit_test.c.o
In file included from /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:17,
from /home/hebasto/dev/secp256k1/secp256k1/src/unit_test.c:31:
/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:19:13: warning: ‘secp256k1_sha256_initialize’ used but never defined
19 | static void secp256k1_sha256_initialize(secp256k1_sha256 *hash);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:20:13: warning: ‘secp256k1_sha256_write’ used but never defined
20 | static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t size);
| ^~~~~~~~~~~~~~~~~~~~~~
/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:21:13: warning: ‘secp256k1_sha256_finalize’ used but never defined
21 | static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out32);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
[18/20] Linking C executable bin/noverify_tests
FAILED: bin/noverify_tests
: && /usr/bin/cc -O2 -g -Wl,--dependency-file=src/CMakeFiles/noverify_tests.dir/link.d src/CMakeFiles/unit_test.dir/unit_test.c.o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o src/CMakeFiles/noverify_tests.dir/tests.c.o -o bin/noverify_tests && :
/usr/bin/ld: src/CMakeFiles/unit_test.dir/unit_test.c.o: in function `testrand_seed':
/home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:29:(.text+0x6c2): undefined reference to `secp256k1_sha256_initialize'
/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:30:(.text+0x6d6): undefined reference to `secp256k1_sha256_write'
/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:31:(.text+0x6e6): undefined reference to `secp256k1_sha256_write'
/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:32:(.text+0x6f1): undefined reference to `secp256k1_sha256_finalize'
collect2: error: ld returned 1 exit status
[19/20] Building C object src/CMakeFiles/tests.dir/tests.c.o
ninja: build stopped: subcommand failed.
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails
In that case, you should add the *_impl.h include in the binary’s top-level file, not here.
I think it helps to think about the *_impl.h files as if they were .cpp files: you would never include them directly in any file. You would only include the headers with the public functions/structs and then link the object files to get their implementations. But since we have a single translation unit here, we include them only in the top-level binary.
Side note: we could also remove this include from the unit test side if needed. Could just add a pointer to the RNG init function and done.
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails:
That may be true, but without #1734, including just hash.h is perfectly correct. Exactly because hash_impl.h is included in tests.c. That's our "unity-build" convention, as correctly described by @furszy.
Right. Including hash_impl.h in unit_tests.c fixes my branch.