https://rules.sonarsource.com/cpp/RSPEC-6214 and https://rules.sonarsource.com/cpp/RSPEC-6183 are very similar. The former is named
"std::cmp_*" functions must be used to compare signed and unsigned values and the latter is named
"std::cmp_*" functions should be used to compare signed and unsigned values. The naming only differs on “must” vs “should”.
However, when I look at the examples in their descriptions, it seems like the difference is that the “must” version will only trigger when the analyzer has determined that for certain there is a negative value value involved. Meanwhile, the “should” version has examples that show that it will trigger when comparing values in general, where the values aren’t known at compile time.
Also the “see also” section at the bottom of these pages have references to the other version, suggesting that the “must” version is more strict and the “should” version is more relaxed. That seems logically that would make sense based on the naming alone, but when looking at the examples it looks like the opposite would be true. If one of these rules only trigger when the signed value can be proven to be negative at compile time, then that seems like a more specialized case. And a rule that triggers also for unknown values should be considered more strict?
I also had a different expectation for that the “must” version of this rule should detect. I thought that it would trigger for cases where one is comparing two values of the same signedness, and suggest to revert back to the normal comparison operators.
Either way, I’m confused about the intended difference between these rules.
There is also another difference between the rules: One is a bug, the other one is a code smell.
You can see the relationship between those rules in the following way:
RSPEC-6214 (the bug) is when we know that a negative value is involved in the comparison.
RSPEC-6183 (the code smell) is just when we know signed and unsigned are mixed, but maybe the signed values are positive, in which case there is no real issue.
So the main criteria to differentiate the rules is the level of confidence we have in the fact that the code as written is broken. Which lead to the notion of strictness: Asking you to change the code even if we are not sure it is really an actual issue is more strict than only asking you to change it only when the issue is real.
Depending on the level of strictness you want in your code (and as a consequence on the number of changes that will have to be performed to comply with the rule), you should select one of these rules, not both.
Is this clarifying the situation?
Yes. That’s sort of what I thought. Maybe it’s the wording that could be improved to be more precise.
I also wonder if it would be beneficial to add another rule to suggest not using these functions when comparing values of the same type/signedness. Because there is a style argument where if the more general tool is used that communicates to the reader of the code that the comparison is between mixed types.
Yes, it’s hard to find good wording… We are open to suggestions!
About your proposed rule, I’m not really convinced:
- You could say that you want to use the new feature only in the cross-signedness case, to show that something tricky happens (this is the point of view that you are sharing),
- Or you might want to use the new feature all the time, so that it becomes a habit and you no longer have to think about it, and to make your code more resilient.
Since there are good arguments in both ways, unless your guideline becomes accepted practice in the community, I’d rather not implement it for now.
Maybe the “must” rule should be rephrased to
"std::cmp_*" functions must be used comparing negative signed values with unsigned values, and just be referred to as a “more specialized case” of the “should” version.
I created a ticket to improve the rule description. You can also follow the PR with the proposed changes (please, tell me if you think they make the situation better).
The PR has been merged, the updated documentation should appear in our next release.
This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.