Add non-null and unused-result warnings for the external API. #101

pull gmaxwell wants to merge 1 commits into bitcoin-core:master from gmaxwell:nonnull_warnings changing 7 files +131 −42

gmaxwell commented at 8:07 PM on November 12, 2014: contributor

GCC (and clang) supports extensions to annotate functions so that their results must be used and so that their arguments can't be statically provable to be null. If a caller violates these requirements they get a warning, so this helps them write correct code.

I deployed this in libopus a couple years ago with good success, and the implementation here is basically copied straight from that.

One consideration is that the non-null annotation teaches the optimizer and will actually compile out runtime non-nullness checks as dead-code. Since this is usually not whats wanted, the non-null annotations are disabled when compiling the library itself.

The commit also removes some dead inclusions of assert.h and introduces compatibility macros for restrict and inline in preparation for some portability improvements.
gmaxwell commented at 8:12 PM on November 12, 2014: contributor

The warnings look like:

warning: ignoring return value of ‘secp256k1_ecdsa_sign_compact’, declared with attribute warn_unused_result [-Wunused-result]

Add non-null and unused-result warnings for the external API.

GCC (and clang) supports extensions to annotate functions so that their
 results must be used and so that their arguments can't be statically
 provable to be null. If a caller violates these requirements they
 get a warning, so this helps them write correct code.

I deployed this in libopus a couple years ago with good success, and
 the implementation here is basically copied straight from that.

One consideration is that the non-null annotation teaches the optimizer
 and will actually compile out runtime non-nullness checks as dead-code.
 Since this is usually not whats wanted, the non-null annotations are
 disabled when compiling the library itself.

The commit also removes some dead inclusions of assert.h and introduces
 compatibility macros for restrict and inline in preparation for some
 portability improvements.

8563713a4f

in include/secp256k1.h:None in b6e87dfc18 outdated

 133 | +  int msglen,
 134 | +  unsigned char *sig64,
 135 | +  const unsigned char *seckey,
 136 | +  const unsigned char *nonce,
 137 | +  int *recid
 138 | +) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5) SECP256K1_ARG_NONNULL(6);

sipa commented at 8:19 PM on November 12, 2014:

argument 6 (recid) is allowed to be NULL.

gmaxwell commented at 8:24 PM on November 12, 2014:

Fixed. I guess I should make the tests test that in another pull.

sipa commented at 10:03 PM on November 12, 2014: contributor

tested ACK
sipa merged this on Nov 12, 2014
sipa closed this on Nov 12, 2014
sipa referenced this in commit f79d80a724 on Nov 12, 2014

Contributors

gmaxwell

sipa