Moves all test-only code into tests and re-enables -Wunused-function.

gmaxwell commented at 1:22 am on September 29, 2015: contributor

This moves a large amount of num_impl code into tests.c because we only ever use it in tests anymore. Likewise for secp256k1_gej_is_valid_var.

The -Wunused-function is only enabled when all modules are turned on and when static precomp is off. This avoids warnings for the few parts of the base library which are only used by modules (or pre-computation).

[Note, the description of this PR has changed significantly since it was opened, because the PR was narrowed to only being unused clean in an all-modules build.]

sipa commented at 11:38 pm on September 29, 2015: contributor

I’m not convinced about this. It creates a continuous burden to maintain exact logic everywhere to determine whether particular top-level logic on top may use a function, for something that is trivially optimized away by a compiler (I would expect). What is the benefit here?

gmaxwell commented at 0:51 am on September 30, 2015: contributor

Perhaps I should have broken it up. The only parts which I think are a potential maintenance burden are the module conditional functionality. Most of this patch is unrelated to the module conditional functionality. I hope you agree that moving the test only code into tests is reasonable?

I was aware the rest was debatable, that is why I specifically called out the cost. I don’t think the burden is that great, as we’ll get yelled at by the compiler.

As far as the benefit, – here are the benefits I see:

We no longer need to suppress this standard warning, which means we’ll find out if we’ve left actually dead code right away (a problem we’ve had multiple times); and people compiling the code base with alternative tool-chains that have not suppressed that warning won’t get a flood of concerning warning messages that make us look like clowns :).

Another benefit is to increase code reviewability by making it easier for people to skip reviewing code they know they’ll never run in production. Admittedly, with the test-only code moved there isn’t much code here, so maybe this one is small.

A last benefit is that not all compilers will reliably optimize out deeply nested uncalled functions. But I don’t actually know how common this is, perhaps we shouldn’t care about it.

Perhaps an alternative would be to move the test-only code, and set things up so that we compile without -Wno-unused-function only when all modules are enabled. Then at least we’ll get warnings for unused code which is unrelated to module selection but avoid peppering things with conditionals. Would you find that preferable?

I really do think the unused function warning is helpful, and I’d prefer to not always be suppressing it– it sometimes even reveals actual bugs (e.g. you meant to call F() but typo and call G() instead, leaving F() unused).

sipa commented at 1:23 am on September 30, 2015: contributor

Yes, my only concern is adding the various complex conditionals all over the place. Moving test-only code, and only testing with no-unused when all modules are enabled is fine by me.

Move secp256k1_ecdsa_sig_recover into recovery module. 03370aa774

Moves all test-only code into tests and re-enables -Wunused-function.

This moves a large amount of num_impl code into tests.c because we only
 ever use it in tests anymore.  Likewise for secp256k1_gej_is_valid_var.

The -Wunused-function is only enabled when all modules are turned on and
 when static precomp is off.  This avoids warnings for the few parts of
 the base library which are only used by modules (or pre-computation).

9ddc815289

gmaxwell renamed this:
~~Eliminate all unused code in all builds and re-enable -Wunused-function.~~
Moves all test-only code into tests and re-enables -Wunused-function.
on Sep 30, 2015

gmaxwell commented at 4:34 am on September 30, 2015: contributor

As you wish!

sipa commented at 8:04 pm on September 30, 2015: contributor

Now that I look at it in more detail, I changed my mind. I don’t think it’s acceptable to move code that touches the num type’s internal representation inside the test code.

gmaxwell commented at 8:26 pm on September 30, 2015: contributor

I wondered when I first created that if you’d think that! I thought of this too.

My counter arguments are:

(1) We are much much more likely to remove num completely (which would then move all of num into test code) than we are to add another num type.

(2) The test code is already saturated with abstraction breaking, twiddling around on the inside of otherwise opaque objects. It has many functions like ge_eq_ and friends that pull back the covers. I don’t see any real problem with this.

If you don’t want to do this.. okay, though I think it’s unfortunate to have a bunch of test only code in the codebase and that it does materially increase the cost of review.

sipa commented at 3:24 pm on October 11, 2015: contributor

(2) Of course to an extent, tests are already breaking abstractions, because they test things at various levels. But not in the way this PR does. secp256k1_ge and secp256k1_gej are already ’exposed’ types, as the type definition is part of their interface - which is fine as long as we see no need for another group implementation (which would be something that for example uses homogeneous coordinates?). Field, scalar, and num are not exposed, and the only way to inspect or mutate them is using functions from the respective module.

I think we can reconsider after #290, which adds a new and incompatible num implementation. Perhaps we can restrict num’s API to only support modular inverses (and perhaps Jacobi symbol), and separate out a naive bignum implementation for use in test. But maybe it makes more sense to keep num fully-fledged as well at that point, as the modinv/jacobi code may end up sharing a lot of code with it anyway.

sipa closed this on Oct 11, 2015

Moves all test-only code into tests and re-enables -Wunused-function. #323