Drake: Some DRAKE_ABORT_UNLESS usages have poor readability

[ggould-tri]

I just reviewed a PR with half a dozen instances of DRAKE_ABORT_UNLESS(something != nullptr). This is something like a quadruple negative "abort is bad... but unless means it's good, but != means it's bad... but nullptr is also bad so it's good..."

This is obviously awful for readability, and makes my head hurt every time I have to puzzle it out.

A much clearer idiom would be DRAKE_REQUIRE_NONNULL(something) or just DRAKE_REQUIRE(something != nullptr).

I propose adding one or both of those macros to our drake assertions.

By filing this issue I understand that I am throwing open the doors of the bikeshed; I'm just interested to see if there's any consensus possible.

[jwnimmer-tri]

I would be happy to replace DRAKE_ABORT_UNLESS with DRAKE_REQUIRE throughout; the former is still a bit wordy, even if it doesn't personally confuse me.

[sherm1]

I still feel that a macro that kills the executable needs to have a violent name, but I agree that ABORT_UNLESS is wordy and already contains a negation. ASSERT already has the right connotation by convention, but also implies "free; goes away in Debug". I have used ASSERT_ALWAYS before which is wordy but positive. That would be my preference here since its meaning is obvious. But if we start a new convention for program death I'd like something as violent as we can find, DRAKE_DEMAND maybe?

A narrower alternative just to address the problem Grant spotted would be to standardize on using the automatic conversion from pointers to bool: DRAKE_ABORT_UNLESS(my_pointer).

[david-german-tri]

How about switching the sign? DRAKE_ABORT_IF or DRAKE_DIE_IF?

(Any of the above, including the status quo, are fine with me. Just thought I'd toss this idea out in case others like it.)

[jwnimmer-tri]

One requirement I'd like to impose is that the new macro and DRAKE_ASSERT have the same sense, so that it's trivial to swap one for the other.

Remember that this is all a giant game because turning off Drake's assertions is a sacred cow, so we need to add a second assertion variant that is always enabled. We just need to (re)agree on the spelling of it.

[jwnimmer-tri]

A modest proposal: DRAKE_ASSERT is always on; DRAKE_EXPENSIVE_ASSERT is toggled off in release mode. (Currently these are DRAKE_ABORT_UNLESS and DRAKE_ASSERT, respectively.)

The gist is to make the thing everyone should use (the former -- always enabled) easy to type and read, and the thing that only systems/framework/inner_loop_73d.cc should ever use (the latter -- disabled in release) more unusual.

[sherm1]

A modest proposal: DRAKE_ASSERT is always on;

I believe we discussed and rejected that idea in the earlier discussion. I'm still opposed to it because I feel that C++ standard assert() has already created the expectation that asserts can be used freely with no Release build cost. If there's a cost, the name shouldn't be an unadorned "assert".

[jwnimmer-tri]

@ggould-tri Are you going to choose the shed color here? This is assigned to three others of us, but my understanding was that you would push this resolution foward?

[ggould-tri]

Yup, I opened this a week ago and was going to come back to it today.

I'll unassign you all and assign myself; you should continue to be notified if your preferences are so configured.

[ggould-tri]

It looks to me from the discussion that sign-reversal would be controversial and renaming to something with the word "assert" would be controversial. A request that the name "sound violent" appears to be uncontroversial (apparently not everyone is so conflict averse as I am in considering "require" fairly threatening...). The use of boolean conversion of pointers appears to be uncontroversial (which would obviate the value of a syntax-lightening "nonnull" variant).

I therefore propose to paint the bikeshed as DRAKE_DEMAND(my_boolean_expr_or_pointer).

[sherm1]

@ggould-tri, to make sure I understand your proposal, I think it is twofold:

1. Rename DRAKE_ABORT_UNLESS to DRAKE_DEMAND
2. Add a style rule that null pointer checks use the automatic pointer-to-bool conversion.

The style change would apply equally to DRAKE_ASSERT(my_pointer) and DRAKE_DEMAND(my_pointer) I presume.

Also, DRAKE_DEMAND(my_pointer != nullptr) seems OK to me since it avoids the double negative. Should we allow it?

[ggould-tri]

Sounds right to me. I propose that use of an explicit != nullptr is permitted but not required (author's discretion) and that I will note all of this in the code conventions guide by clarifying the current stanza about asserts in the "Additional Rules" section.

[jwnimmer-tri]

While I will miss the citation to Eiffel that DRAKE_REQUIRE would give us, I can certainly live with DRAKE_DEMAND. Certainly it's a verb more consistent with the _SHOUTING_ we get from macros.