Improve assertion macro names

[sethrj]

Coarse summary of a conversation with @pcanal at #95 (comment) and #95 (comment):

• REQUIRE/CHECK/ENSURE/INSIST (which derive from Design-by-Contract nomenclature and their implementation and extension in LANL's Draco C++ support library ) are unscoped macros: we should prefix with CELER_.
• The downside of longer macros is they become unwieldy.
• The meaning of the four, and the stark difference in INSIST, isn't obvious from the names (only in context of the code).

The difference between INSIST and CHECK is that INSIST

• throws a RuntimeError instead of DebugError (i.e. it's intended for conditions that aren't the programmer's fault),
• does not get disabled when CELERITAS_DEBUG is false, and
• has a second argument that is a verbose error message.

[sethrj]

Perhaps we should adopt (but depluralize) the "expects"/"ensures" nomenclature of the abortive C++20 proposal for contract checking, with the standard assert for internal assertions, and then I'm still open to a suggestion on the black sheep that is INSIST:

• CELER_EXPECT: precondition assertions
• CELER_ASSERT: general internal assertions
• CELER_ENSURE: postcondition assertions
• CELER_ASSERT_UNREACHABLE: if debug, throw an assertion; if non-debug, tell the compiler the code point is unreachable for optimization/warning suppression
• CELER_INSIST: handle (foreseeable) unrecoverable runtime error

I think it's ok for CELER_ASSERT_UNREACHABLE to be longer and unwieldy because it doesn't take any arguments and it's relatively uncommon. I would write it as CELER_ASSERT(unreachable) if it weren't for the fact that it evaluates to CELER_UNREACHABLE (which is a macro that in GCC/clang is __builtin_unreachable()) which debugging is off.

[pcanal]

(but depluralize)

Isn't the C++20 proposal using verb conjugation (as in 3rd person present tense) rather than plural. Such that

CELER_EXPECTS( state == something );

reads
Celeritas expects that the state variable is equal to something?

[sethrj]

Aha, I'm sure you're right. The reason I suggested changing it is that usually I expect functions/macros to be imperative verbs since they're an invocation. The C++20 proposal is an annotation (a la [[noreturn]]) so I think it's OK to have a different part of speech.

[pcanal]

In that set, I would say that:

• CELER_REQUIRES: handle (foreseeable) unrecoverable runtime error (or CELER_FATAL_ERROR)

sorta makes sense. As "requiring" as a stronger connotation as "expecting".

[pcanal]

For me "Insist" as an odd slant as I read it closer to the meaning of "to maintain in a persistent or positive manner ". I.e. rather than requiring the condition to be true, it feels that INSIST would try harder to make it true.

[sethrj]

Well, I that's that's the intent though perhaps with a different twist on it -- to me, the "persistent" aspect of it is that it doesn't get turned off when debugging is disabled.

[pcanal]

I can see that but while reading the code (without long exposure to that macro), it does not read that way unless we spell it closer to CELER_INSIST_ON_ASSERTING_FOR ... i.e. what "insist" is insisting on is really meta in the CELER_INSIST (it almost requires that the reader assumes that all macro (upper cases entities) are assertion types ... [and that's difficult since it is not really the case]

[pcanal]

And I don't think CELER_ERROR(condition, failure_msg) parses as well as another "assurance"-type verb such as CELER_INSIST(condition, failure_msg). It doesn't convey that it's only an error if the condition is false.

I agree with this conclusion. 'Error' would only make sense in the style if (!condition) Error(...) or if we use a prefix like _IF, for example: CELER_FATAL_ERROR_IF(failure_condition,...)

[pcanal]

Should we tackle this now, or add this to the list of things we can/should do to improve the codebase especially as an external-facing library? 😬

I would recommend to go ahead now. There is less code to update, the 'previous one' are less ingrained that they will be by then (and/or we have more time to get used to the new one) and the code base is small enough that we can even imagine forgoing the backward compatibility stage and ask for the PR to be updated (sed should work fine here to update the code).

[sethrj]

Yeah, agreed.

[sethrj]

OK @pcanal how about this. I think we should use imperative tense because these are function-like calls rather than annotations, and they're in the spirit of the assert and static_assert macro/keywords.

• CELER_EXPECT(condition): precondition assertions
• CELER_ASSERT(condition): general internal assertions
• CELER_ENSURE(condition): postcondition assertions
• CELER_ASSERT_UNREACHABLE(): if debug, throw an assertion; if non-debug, tell the compiler the code point is unreachable for optimization/warning suppression
• CELER_RUNTIME_ASSERT(condition, streamable_message): verbose, always-on checking that raises a runtime error if the condition fails. Like static_assert, it takes two arguments: condition, and explanation. Also like static_assert it can't be disabled.

[pcanal]

I think we should use imperative tense

I still feel the present tense reads a little better but the precedents you pointed out make senses (in favor of imperative tense).

CELER_RUNTIME_ASSERT

Not my favorite but I think it would do (and is better than another version of insist :) : CELER_NDEBUG_ASSERT)

[sethrj]

So you're OK with CELER_RUNTIME_ASSERT or do you prefer CELER_NDEBUG_ASSERT? The latter might be confused for something that's only on when -DNDEBUG, which isn't used for any of our assertion macros).

[pcanal]

I was not seriously proposing CELER_NDEBUG_ASSERT. I still prefer a non-assert naming (assert tend to be associated with being using only with NDEBUG not defined), for example CELER_THROW_IF(failure_condtion, streamable_message).

[sethrj]

Gotcha, I understand with the assert -- though since static_assert is on regardless of NDEBUG I would argue it's OK.

I don't want to use IF since it negates the predicate (going from success condition to failure condition) -- and the use of THROW makes it look to my eyes that the second argument should be an assertion class, like Google Test's EXPECT_THROW(expression, assertion_type).

By the way, not to drag @tmdelellis into this, but the check/require/ensure nomenclature is from the Eiffel programming language implementation of design-by-contract assertions. I don't see Insist in there, though, so that might be a LANL extension.

I think to keep it simple the insist-like macro should be just a single imperative verb (scoped by CELER_):

• CELER_VERIFY?
• CELER_VALIDATE?

I would be amenable to another word if it really makes the difference between understanding the macro's action and not... but I'm really not seeing that much improvement in clarity from most of the options we've discussed.

[sethrj]

OK per our discussion, the final remapping is:

Original	New
REQUIRE	CELER_EXPECT
CHECK	CELER_ASSERT
ENSURE	CELER_ENSURE
CHECK_UNREACHABLE	CELER_ASSERT_UNREACHABLE
INSIST	CELER_VALIDATE

I'll make these changes in a PR tonight after #104 gets merged.