test: enable -Wunused-function in test suite (Fix #1831) #1865

pull kallal79 wants to merge 1 commits into bitcoin-core:master from kallal79:fix-unused-function-1831 changing 5 files +53 −38

kallal79 commented at 8:04 PM on June 8, 2026: none
This PR addresses issue #1831 by enabling the -Wunused-function compiler warning within the test suite.

Currently, -Wno-unused-function is passed globally to disable warnings about unused functions, making it too easy to write a test case but forget to actually call it. To catch untested helper functions safely, this PR uses GCC/Clang pragmas scoped strictly to the body of the test files.

Changes Made:
- Added #pragma GCC diagnostic warning "-Wunused-function" directly after the #include statements in src/tests.c, src/tests_exhaustive.c, src/ctime_tests.c, and src/unit_test.c. Fixes #1831
in src/ctime_tests.c:44 in fa8e1c0e89 outdated
```
  39 | @@ -40,6 +40,11 @@
  40 |  #include "../include/secp256k1_ellswift.h"
  41 |  #endif
  42 |  
  43 | +#if defined(__GNUC__)
  44 | +# pragma GCC diagnostic push
```
real-or-random commented at 7:57 AM on June 9, 2026:

A push without a pop doesn't makes a lot of sense. I think the cleanest thing is to add a pop at the end of each test file.

real-or-random commented at 8:09 AM on June 9, 2026:

Also our macro SECP256K1_GNUC_PREREQ(4, 2) would be preferable over defined(__GNUC__).

(This is maybe questionable when it comes to these old compiler versions but as long as we have the macro, we should use it.)

hebasto commented at 2:45 PM on June 10, 2026:

While the -Wno-unused-function diagnostics has been available since GCC 3.0, the support for #pragma GCC diagnostic was introduced only in GCC 4.2, and the push/pop variants in GCC 4.6. Since this PR uses diagnostic push, the guard should be SECP256K1_GNUC_PREREQ(4, 6). Note that this check evaluates to false under Clang, which masquerades as GCC 4.2.x, so an additional defined(__clang__) check is required.

real-or-random commented at 8:01 AM on June 12, 2026:

(Oh, I had marked this conversation resolved already, so I missed your comment @hebasto.)

You're right. I suggested the wrong version here. (I trusted ChatGPT 5.4 but apparently you can't even trust it for these simple things... -.-) I think we should really get rid of the SECP256K1_GNUC_PREREQ macro. @kallal79 For now, I suggest to change this back to what you had proposed initially (#if defined(__GNUC__)) because this will work both for GCC and Clang. Sorry for the back and forth...
real-or-random commented at 7:57 AM on June 9, 2026: contributor

Thanks for the PR. :)

Concept ACK
real-or-random added the label assurance on Jun 9, 2026
real-or-random added the label meta/development on Jun 9, 2026
real-or-random added the label tweak/refactor on Jun 9, 2026
real-or-random commented at 11:12 AM on June 10, 2026: contributor

Thanks for the quick update.

ACK mod that this should be squashed

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

We follow the same workflow as Bitcoin Core here, see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits for background.
real-or-random commented at 11:13 AM on June 10, 2026: contributor

@hebasto Want to review this?
hebasto commented at 2:10 PM on June 10, 2026: member

~From #1831#issue-4010176205:~ ~> ... we should enable -Wunused-function at least in the test suite.~

~Could this be enabled as well?~
hebasto commented at 3:11 PM on June 10, 2026: member

Concept ACK.

An alternative approach would be appending -Wunused-function to the compiler flags when building test targets. However, that would require changing and then maintaining for both build systems.

From #1831#issue-4010176205:

It's too easy to add a test but forget to call it.

Tested this scenario. Works as expected.
real-or-random commented at 7:48 AM on June 12, 2026: contributor

@kallal79

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

Just checking: Did you see this request? (I saw that you gave a few thumbs-ups in later comment.)
hebasto commented at 7:51 AM on June 12, 2026: member

@kallal79

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

Just checking: Did you see this request? (I saw that you gave a few thumbs-ups in later comment.)

And #1865 (review)?
test: enable -Wunused-function in test suite (Fix #1831) 6659bc25c0
kallal79 force-pushed on Jun 12, 2026

Contributors

kallal79

real-or-random

hebasto

Labels

assurance tweak/refactor meta/development

Linked (view graph)

#1831 Enable -Wunused-function #1868 Remove SECP256K1_GNUC_PREREQ?

test: enable -Wunused-function in test suite (Fix #1831) #1865

Changes Made: