Possible false positive

[Antonio Lotti]

Hello.
I think I found a false positive while using CppCheck on MuseScore source code.
It is triggered by the use of an overloaded comparison in a condition involving an implicit conversion.
Attached you can find a minimal example.
CppCheck v2.2 prompts two warnings:
- noExplicitConstructor
- knownConditionTrueFalse

<error id="knownConditionTrueFalse" severity="style" msg="Condition &amp;#039;d==TDuration::DurationType::V_16TH&amp;#039; is always false" verbose="Condition &amp;#039;d==TDuration::DurationType::V_16TH&amp;#039; is always false" cwe="570" hash="12217776682951756754">
            <location file="prova.cpp" line="30" info="Condition &amp;#039;d==TDuration::DurationType::V_16TH&amp;#039; is always false"/>
            <location file="prova.cpp" line="28" info="Assuming that condition &amp;#039;d&amp;gt;=TDuration::DurationType::V_EIGHTH&amp;#039; is not redundant"/>
        </error>

The "knownConditionTrueFalse" in this case is not correct, since the comparison is done via the overloaded ">=" operator by implicitly converting the DurationType to a TDuration. Therefore TDuration d = TDuration(TDuration::DurationType::V_16TH) is for example smaller (with the overloaded operator) than TDuration::DurationType::V_EIGHTH (implicitly converted to a TDuration).
If the conversion in the condition is made explicit, then the "knownConditionTrueFalse" warning disappears.

Ciao,
ABL

[Daniel Marjamäki]

ok.. so the overloaded operator will inverse the comparison. This:

if (d >= TDuration::DurationType::V_EIGHTH)

Means:

if (d._val <= TDuration::DurationType::V_EIGHTH)

We do not handle overloaded operators 100% I am afraid and we assume they are logically consistent with standard behavior. But well I think we should handle them better..

[Daniel Marjamäki]

I wonder.. don't you think it would be better to be consistent with standard behavior? Is this intentional to invert the comparison?

[Antonio Lotti]

I think this inversion was done to make it easier to append shorter durations to the DurationType enum (so that the previous values in the list are not touched), while at the same time have a comparison between TDurations (i.e note or rest time durations) looking more "natural" (i.e. a shorter TDuration being smaller than a longer TDuration in comparison conditions). Indeed, three or four years ago, the DurationType enum was enlarged to include also 512th and 1024th, which were not initially present.

By the way, since we know it is just a false positive, we can simply ignore this warning.

Have a nice day,
Antonio

[Daniel Marjamäki]

I can agree that technically it's a false positive. So it is clearly unfortunate.

By the way, since we know it is just a false positive, we can simply ignore this warning.

ok thanks!

I hope you are not discouraged. Please feel free to report other false positives that you might have.