mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.25k stars 1.24k forks source link

feat(assert): add general assertion that always executes its condition #13239

Closed Swiftb0y closed 1 month ago

Swiftb0y commented 1 month ago

Its purpose is used to check the value of conditions whose sideeffects are critical to the program. this makes it possible to avoid code such as

[[maybe_unused]] bool successful = moveToThread(m_pThread.get());
DEBUG_ASSERT(successful);

and instead write

ASSERT(moveToThread(m_pThread.get()));

I personally think we should not compile out the check in release builds as the branch is very likely to get predicted correctly and thus is almost free.

daschuer commented 1 month ago

I don't like to have any assert() when doing a gig! There is no reason to artificially crash if no developer is available.

We have discussed this when introducing our assert.h and at that time we consider the only reason for such a release assert to protect user data from damage. But it turns out that we where able to graceful handle all such situations.

daschuer commented 1 month ago

But anyway, this PR is redundant, because we already have RELEASE_ASSERT() for that purpose.

Swiftb0y commented 1 month ago

I don't like to have any assert() when doing a gig! There is no reason to artificially crash if no developer is available.

I fully agree. This assert does not crash. Instead it only prints to the console and as I said, the primary purpose is to still execute the condition because of its sideffect. Thats also why this is different from RELEASE_ASSERT. see https://github.com/mixxxdj/mixxx/pull/11407#discussion_r1594206453

daschuer commented 1 month ago

OK so the macro name ASSERT is misleading.

In these case we use normally VERIFY_OR_DEBUG_ASSERT

bool successful = moveToThread(m_pThread.get());
VERIFY_OR_DEBUG_ASSERT(successful) {
    // recover gracefully 
}

or just log a warning: https://github.com/mixxxdj/mixxx/blob/8f647908af460e53fe8a860b6ce0964a93a55112/src/engine/enginesidechaincompressor.cpp#L69-L71

This has the issue that the warning is not printed when assertions are fatal and no line number is printed when only the verify is done.

Did you intend to fix that? Something like

VERIFY_OR_DEBUG_ASSERT(m_compressRatio >= 0, "Programming error, below-zero compression detected.") { 
} 
Swiftb0y commented 1 month ago

you are right, at least for this concrete usecase, a regular VERIFY_OR_DEBUG_ASSERT is the better solution.