Murdock results

✔️ PASSED

36a34e5 core/assert: mark failed condition unreachable with NDEBUG

Artifacts

• Documentation preview

bors try

Making the functions inline allows to git bisect seamlessly as the the code still compiles without b8e4965.

But I can drop it.

maybe @miri64 or @kaspar030 wants to a (second) look

This is somewhat similar to #16375 (before the drop of assert_ng()). However, in addition to fix all the compilation issues and fixing the

void foo(int bar)
{
#NDEBUG
    (void)bar
#endif
    assert(bar == 42);

boilerplate this also improves the binaries.

I'm in favor of this. It does change the semantics of assert(), but code that does use assert() correctly will not be affect. It will only fix bugs such as

    assert(spi_acquire(...) == SPI_OK);

to still work with NDEBUG correctly. I'd argue that changes in behavior of buggy code are not that much of a concern, as we should just fix the bugs anyway.

We do no longer have a cppcheck pass that would prevent statements with side-effects in assert(). It would be nice to have something else that enforces side-effect free statements in assert(), so that our code would not start relying on our custom assert() semantics.

If you add a bit of documentation that explains how our assert() would deviate from the C standard and why we do so, I'd give a second ACK.

I think something such as

#define LOG_DEBUG(...) assert(printf(__VA_ARGS__) > 0)

would be actually legitimate, but insane. But I cannot come up with code that is sane and bug-free that would notice the deviation from the C standard here.

I extended the documentation in assert.h

Wouldn't this change also introduce additional if statements for productive code?

It might introduce additional statements; as long as the assert is simple, it typically won't (because the unreachable will just narrow down the legal values), but if the assert includes a call outside the current compilation unit, that call suddenly will be made.

In my perception assert() must not (under no circumstances) result in any code when NDEBUG is set.

In my perception assert() must not (under no circumstances) result in any code when NDEBUG is set.

Assumption about generated code and the C language don't mix well together.

The C compiler is allowed to do crazy things like taking the hash value of the source file as seed for a random number generator that is used for statistical analysis (e.g. which code path is taken more often 200 randomly chosen inputs) and optimize for that. Fixing a typo in a comment would then result in possibly better or worse machine code (in terms of code size, performance). Both versions would however be show the same observable behavior and, thus, would be correct.

In my perception assert() must not (under no circumstances) result in any code when NDEBUG is set.

Assumption about generated code and the C language don't mix well together.

Unless it's part of a specification. The POSIX doc says:
"Forcing a definition of the name NDEBUG, either from the compiler command line or with the preprocessor control statement #define NDEBUG ahead of the #include <assert.h> statement, shall stop assertions from being compiled into the program."

The Linux man page for instance states:
"If the macro NDEBUG is defined at the moment <assert.h> was last included, the macro assert() generates no code"

superseded by merged #19354