ARRAY_SIZE macro
#1824
This PR is another tiny improvement found while working on #1765, with the goal to avoid code repetition.
The ARRAY_SIZE macro definition is pretty wide-spread in C projects and e.g. matches the one used in the Linux Kernel (without the additional check to reject pointers, as we would need GNU C for that, see e.g. https://stackoverflow.com/a/19455169; not sure if a useful counterpart exists that only relies on C89). Replacement instances were identified via $ git grep sizeof.*/.*sizeof.
The macro definition matches the one used in Linux, see e.g.
https://github.com/torvalds/linux/blob/9702969978695d9a699a1f34771580cdbb153b33/include/linux/array_size.h#L11
(without the additional check rejecting pointers, as we would need
GNU C for that, see e.g. https://stackoverflow.com/a/19455169)
I think overall the ARRAY_SIZE macro makes the codebase overall cleaner.
I’m just unsure because we can pass pointers into ARRAY_SIZE and have it silently pass.
I would add a comment above the macro to say to avoid using pointers, so if someone were to use the macro, they would be aware.
(without the additional check to reject pointers, as we would need GNU C for that, see e.g. stackoverflow.com/a/19455169;
GNU C is not an issue if we sandwich it in a #if SECP256K1_GNUC_PREREQ. A check that works only on GCC will be enough for us; we’ll always test on GCC.
And we don’t need BUILD_BUG_ON_ZERO because we have a similar STATIC_ASSERT macro. (Though I really like the implementation of BUILD_BUG_ON_ZERO and that it is an expression and not a statement; our macro is a statement that cannot be used anywhere BUILD_BUG_ON_ZERO can be used.)
(without the additional check to reject pointers, as we would need GNU C for that, see e.g. stackoverflow.com/a/19455169;
GNU C is not an issue if we sandwich it in a
#if SECP256K1_GNUC_PREREQ. A check that works only on GCC will be enough for us; we’ll always test on GCC.
Ah good point, didn’t consider that. I found that the needed typeof keyword is only available if the GNU extensions are explicitly enabled via e.g. -std=gnu90 (as opposed to -std=c90 what we use) though, but the alternative keyword __typeof__ seems to work.
And we don’t need
BUILD_BUG_ON_ZERObecause we have a similarSTATIC_ASSERTmacro. (Though I really like the implementation ofBUILD_BUG_ON_ZEROand that it is an expression and not a statement; our macro is a statement that cannot be used anywhereBUILD_BUG_ON_ZEROcan be used.)
Isn’t it being an expression necessary for most our use cases? We primarily take use of it in the condition of for loops (i.e. for (i = 0; i < ARRAY_SIZE(...); i++), I assume STATIC_ASSERT wouldn’t work there.
Will take a deeper look soon and put the PR into draft state in any case until we figure out how to best add a “reject pointers” check.
Isn’t it being an expression necessary for most our use cases? We primarily take use of it in the condition of for loops (i.e.
for (i = 0; i < ARRAY_SIZE(...); i++), I assumeSTATIC_ASSERTwouldn’t work there.
Good point. Then we’ll need to port the macro.
Or perhaps we should just drop -std=c90, which would give us _Static_assert and make some of the C hacks obsolete (see https://github.com/torvalds/linux/blob/75a452d31ba697fc986609dd4905294e07687992/include/linux/compiler.h#L203). We could do a preprocessor check on __STDC_VERSION__ to determine if we have _Static_assert.
The reason we pass -std=c90 is to make sure that we only use C90 features. We could drop it and add a single CI job to test compilation with -std=c90 for that purpose.
Or perhaps we should just drop -std=c90, which would give us _Static_assert and make some of the C hacks obsolete (see https://github.com/torvalds/linux/blob/75a452d31ba697fc986609dd4905294e07687992/include/linux/compiler.h#L203). We could do a preprocessor check on STDC_VERSION to determine if we have _Static_assert.
The following works in my environment with gcc 14.2.0, after bumping the standard to C11 (needed to get _Static_assert) and removing -pedantic (needed to avoid a struct has no members [-Wpedantic] warning in the BUILD_BUG_ON_ZERO macro): https://github.com/theStack/secp256k1/commit/d02a5de4deb1facb48b93d8c1807d7d8113298bf
Can be tested by e.g.
0--- a/src/modules/schnorrsig/bench_impl.h
1+++ b/src/modules/schnorrsig/bench_impl.h
2@@ -52,6 +52,7 @@ static void run_schnorrsig_bench(int iters, int argc, char** argv) {
3
4 data.ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
5 data.keypairs = malloc(iters * sizeof(secp256k1_keypair *));
6+ i = ARRAY_SIZE(data.keypairs); /* this should fail */
7 data.pk = malloc(iters * sizeof(unsigned char *));
8 data.msgs = malloc(iters * sizeof(unsigned char *));
9 data.sigs = malloc(iters * sizeof(unsigned char *));
It seems though that even without the must_be_array addition, the compiler is able to detect misusage of ARRAY_SIZE, and there is a dedicated warning category (-Wsizeof-pointer-div):
0/home/thestack/secp256k1/src/util.h:191:39: warning: division ‘sizeof (const secp256k1_keypair **) / sizeof (const secp256k1_keypair *)’ does not compute the number of array elements [-Wsizeof-pointer-div]
1 191 | # define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
2 | ^
3/home/thestack/secp256k1/src/modules/schnorrsig/bench_impl.h:55:9: note: in expansion of macro ‘ARRAY_SIZE’
4 55 | i = ARRAY_SIZE(data.keypairs); /* this should fail */
5 | ^~~~~~~~~~
after bumping the standard to C11 (needed to get
_Static_assert)
What if you don’t pass -std at all?
And removing
-pedantic(needed to avoid astruct has no members [-Wpedantic]warning in theBUILD_BUG_ON_ZEROmacro): theStack@d02a5de
I guess you could just add a member.
after bumping the standard to C11 (needed to get
_Static_assert)What if you don’t pass
-stdat all?
And removing
-pedantic(needed to avoid astruct has no members [-Wpedantic]warning in theBUILD_BUG_ON_ZEROmacro): theStack@d02a5deI guess you could just add a member. @real-or-random: Good points. Removed the explicit setting of a C standard version (for some reason I assumed that setting
CMAKE_C_STANDARDis mandatory in CMake, but apparently it isn’t) and added a dummy member in the struct, so the-pedanticflag can be kept: https://github.com/theStack/secp256k1/commit/62947de7800a39f86ec57c12949368e6c9031b8d
I wonder if its worth it implementing this? Seems to be quite some code for something that can be detected by modern compilers anyways (though I didn’t look into the details of -Wsizeof-pointer-div, maybe it doesn’t catch as much).
I wonder if its worth it implementing this? Seems to be quite some code for something that can be detected by modern compilers anyways (though I didn’t look into the details of
-Wsizeof-pointer-div, maybe it doesn’t catch as much).
Oh, I wasn’t aware of the warning at all. Seems to work: https://godbolt.org/z/vxehGPznG
Yes, then the warning is better… I mean, there could be specific circumstances under which this won’t work, but this would be rather strange because the expression will always look the same if generated by the macro.
@theStack Are you okay with this conclusion? And, if yes, is the PR ready from your side? (Sorry, I shouldn’t have marked it ready for you.)
Yes, I agree with the conclusion and think the PR is ready 👌 Thanks for verifying on godbolt! (and no worries, I think unmarking draft state was fine)