Memory fix #11331
Memory fix #11331
Conversation
| @@ -38,6 +38,7 @@ ControlDoublePrivate::ControlDoublePrivate() | |||
| Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX), | |||
| // default CO is read only | |||
| m_confirmRequired(true) { | |||
| m_value.setValue(0.0); | |||
There was a problem hiding this comment.
This silences just the valgrind warning and make the bug itself invisible for code analysis. The root cause must be somewhere in the use of the overloaded classes.
There was a problem hiding this comment.
This is the value of the constant default ControlObject that is used whenever there is no real control object.
This has been introduced to get around many null checks when dealing with control objects.
The original issue happens here:
mixxx/src/widget/wwidgetstack.cpp
Line 157 in 7d69c32
There was a problem hiding this comment.
Than you must set the correct value (might be 0 here), in the class, where the original issue occured.
Otherwise you make Valgrind blind for bugs of the same kind in other classes. And it depends on the context, if 0 is the correct value there.
There was a problem hiding this comment.
We only have a single Default Control object for all, and it is const:
Line 192 in 7d69c32
So it should have a reasonable default of 0 instead of a random value.
There was a problem hiding this comment.
The only important point is: Never initialize with dummy values!
Which implies to not initialize in the base class, do this in /src/widget/wwidgetstack.cpp instead.
Otherwise Valgrind will be blind for "Use Of Uninitialiezd Variable" bugs introduced in future.
Valgrind can not distinguish between initialization with dummy values and intialize with real values. But it can detect code pathes, were the variable is used before initialization. Therefore always initializes with the real value, which is correct in the specific context - than Valgrind, ASAN and statical code analysis tools like cppcheck will detect the real bugs in other paths.
There was a problem hiding this comment.
I can confirm the rule in general. But in this case the whole ContrObject is a singleton read only dummy. There is no point of a business logic value initialization and you demand is not possible here.
The control object system has already warnings if a control uses the default CO unintentionally.
There was a problem hiding this comment.
If there are already warnings in place, this is of cause Ok here.
|
OK, than this is ready for merge. |
There was a problem hiding this comment.
Thank you, waiting for @JoergAtGithub to resolve his review and merge.
|
Thank you! |
Some more memory fixes discovered by Valgrind.