ControlDoublePrivate: change warnings to debug assertions #4473
Conversation
Be-ing
force-pushed
the
alias_debug_assert
branch
from
cb0888f
to
7f55425
Compare
| @@ -109,14 +109,14 @@ void ControlDoublePrivate::insertAlias(const ConfigKey& alias, const ConfigKey& | |||
| MMutexLocker locker(&s_qCOHashMutex); | |||
|
|
|||
| auto it = s_qCOHash.constFind(key); | |||
| if (it == s_qCOHash.constEnd()) { | |||
| qWarning() << "WARNING: ControlDoublePrivate::insertAlias called for null control" << key; | |||
| VERIFY_OR_DEBUG_ASSERT(it != s_qCOHash.constEnd()) { | |||
There was a problem hiding this comment.
It doe not work that way around, because the warning is no longer issued after that assertion.
You can find a working pattern here:
Line 140 in 625cebd
There was a problem hiding this comment.
Please follow the link to see the whole solution.
There was a problem hiding this comment.
The proposed pattern is cumbersome and only used rarely in the code base. We didn't agree on it.
During development triggering an assertion is intended. In the release version assertions are disabled and the warning is printed instead in case we missed an occurrence. I remember to have used this pattern frequently.
There was a problem hiding this comment.
TL;DR I disagree and propose to use VERIFY_OR_DEBUG_ASSERT + qWarning() as is.
There was a problem hiding this comment.
You are right that the linked solution is a bit cumbersome due to the two instances giving a message. Maybe it is worth to consider a macro that allows to give more context in a failed assertion case.
But anyway this PR prints:
critical [Main] DEBUG ASSERT: "it != s_qCOHash.constEnd()" in function static void ControlDoublePrivate::insertAlias(const ConfigKey&, const ConfigKey&) at /home/sperry/workspace/i18n/src/control/control.cpp:112
in case of assertions enabled which is the case during development and
warning [Main] cannot create alias for null control "[EqualizerRack1_[Channel4]_Effect1]" , "typo"
in a release builds
With the linked pattern we would have have something like
warning [Main] cannot create alias for null control "[EqualizerRack1_[Channel4]_Effect1]" , "typo"
critical [Main] DEBUG ASSERT: "false" in function static void ControlDoublePrivate::insertAlias(const ConfigKey&, const ConfigKey&) at /home/sperry/workspace/i18n/src/control/control.cpp:112
Which is IMHO clearly an improvement but more chatty than necessary.
After reading your comment, I have realized that this issue is not that important here, because we do not introduce aliases frequently. In favor of deescalation, I will merge this now.
No description provided.