Redis defines its own assert macro differently from <assert.h> - redis macro is active even when NDEBUG is defined.
Even with these semantics, redis contributors historically avoided wrapping actionable code within assert statements, i.e. assert was used only for testing invariants and not for executing functions.
However, I see that listpack.c has been using assert more frivously.
Please consider separating statement execution from invariant checking or using a macro with a different name to avoid possible confusion.
Comment From: chenyang8094
I think they should all be replaced with serverAssert.
Comment From: madolson
BTW, assert is aliased to serverAssert() if imported through redisassert.h.
I agree we should probably use a different name though, it's weird that we use a drop in replacement with different behavior. I don't think it's a high priority though. The intention is that we always build redis with assertions running, so I think it's fine to leave function calls within assertion calls, since we are asserting the result of the call.
Comment From: oranagra
@romange thank you. but even if we fix listpack and a few others, we have no guarantee that changing the assert macro to be skipped on NDEBUG will not lead to bugs. the only way to do that is either review all the code, and / or add tests that run in that mode. so changing that code now has no value, could be a wasted effort. but point taken, maybe we'll try to avoid adding more of these. thanks for reaching out though.