Current situation
The purpose of VERIFY_CHECK(cond) is to assert cond in test builds, i.e., when VERIFY is defined. While cond is supposed to have no side effects, VERIFY_CHECK will evaluate it even in production builds, to make sure that no side effects are masked in production builds.
In the case of simple cond, we can rely on the ability of the compiler to detect that cond has no side effects and to omit its evaluation entirely. However, many recently introduced cond are neither simple nor have negligible computation costs. In this case, we usually wrap VERIFY_CHECK(cond) in an #ifdef VERIFY block, which forces the evaluation to be omitted in production, at the cost of foregoing the aforementioned “side-effect safety” of VERIFY_CHECK. [1]
On top of this, we now have (full) functions like secp256k1_fe_verify, which internally perform VERIFY_CHECKs.
I don’t think anything we currently do is wrong, but sometimes the borders are a bit arbitrary, and with a mix of #ifdef VERIFY and no #ifdef VERIFY, and a mix of uppercase and lowercase, also readability and consistency suffer.
Options
When it comes to forcing evaluations to be omitted, we can either keep the current convention and force omitting on a case-by-case basis. Alternatively, we could just always force omitting, e.g., by defining VERIFY_CHECK to be empty in production. That will keep things simple and save us the decision of whether to force omitting. The comment in #904 (comment) appears to agree with this.
When it comes to readability, I think that #ifdef blocks tend to clutter the code. If we redefine VERIFY_CHECK to be empty in production, we could omit many #ifdef lines. Even if we keep two types of VERIFY_CHECKs, I suggest introducing a separate macro instead to replace #ifdef.
One remaining purpose of the #ifdefs is to highlight the test code blocks, e.g., when we call secp256k1_fe_verify in field_impl.h. But then we could just make these function names uppercase. And if we want blocks/grouping for better readability, we can simply use empty lines instead of #ifdef lines.
So I suggest
- redefining
VERIFY_CHECKto be empty in production - renaming
_checkfunctions to uppercase.
And while we’re touching this anyway, we could consider renaming “check” to “assert”, which is a more precise term. (In fact, if we redefine VERIFY_CHECK to be empty in production, we have almost reimplemented assert.h…)
Does this make sense?
[1] One idea was to improve this situation was to introduce a C hack that will tell us whether a compiler is able to determine that a check has no side effects, but I agree with #904 (comment) that this is not the way to go. In addition to the points brought up by @sipa, relying on the compiler is just very brittle, with optimizations depending on compiler versions and flags.
