[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

Fri Mar 3 09:50:03 PST 2023

hazohelet added a comment.

In D142800#4165090 <https://reviews.llvm.org/D142800#4165090>, @aaron.ballman wrote:

> In D142800#4104241 <https://reviews.llvm.org/D142800#4104241>, @hazohelet wrote:
>
>>> Also, why are these diagnostics off by default? Do we have some idea as to the false positive rate?
>>
>> As for the false positive rate, I have checked for instances of this warning in the codebases for 'oneapi-src/oneTBB', 'rui314/mold', and 'microsoft/lightgbm', but did not find any new cases.
>
> Thank you for doing the extra testing! Sorry for the delayed response, this review fell off my radar for a bit.
>
>> I also ran a test on  'tensorflow/tensorflow' using gcc '-Wparentheses' and found six lines of code that trigger the new diagnostic. They all relate to checking whether `x` and `y` have the same sign using `x > 0 == y > 0` and alike. I tried to build with tensorflow using clang, but I stumbled upon some errors (for my poor knowledge of bazel configuration), so here I am using gcc.
>
> Thank you for reporting this, that's really good information.
>
>> I set the diagnostic disabled by default for compatibility with gcc. Considering the test against tensorflow above, it would be too noisy if we turned on the suggest-parentheses diagnostic by default (From my best guess, it would generate at least 18 new instances of warning on the tensorflow build for the six lines).
>> However, in my opinion, it is reasonable enough to have the diagnostic on chained relational operators enabled by default. The following is an excerpt from the 'Existing Code in C++' section of the proposal document of the introduction of chaining comparison for C++17.
>>
>>> Overall, what we found was:
>>>
>>> - Zero instances of chained arithmetic comparisons that are correct today. That is, intentionally using the current standard behavior.
>>> - Four instances of currently-erroneous arithmetic chaining, of the assert(0 <= ratio <= 1.0); variety. These are bugs that compile today but don’t do what the programmer intended, but with this proposal would change in meaning to become correct.
>>> - Many instances of using successive comparison operators in DSLs that overloaded these operators to give meaning unrelated to comparisons.
>>
>> Note that the 'chaining comparisons' in the document are
>>
>>> - all `==`, such as `a == b == c == d`;
>>> - all `{<, <=}`, such as `a < b <= c < d`; and
>>> - all `{>, >=}` (e.g., `a >= b > c > d`).
>>
>> URL: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0893r0.html
>>
>> Although this paper is five years old now, I think we can conclude chaining relational operators are bug-prone and should be diagnosed by default.
>> Also, reading the document above, I think it would be reasonable to suggest adding '&&' in `a == b == c` case, too.
>
> I tend to agree -- I was searching around sourcegraph (https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29%5Cs*%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29&patternType=regexp&sm=1&groupBy=repo) and I can't find a whole lot of evidence for chained operators outside of comments.

Thanks for the review and the sourcegraph search!
I added a new warning flag `-Wchaining-comparisons` that is enabled by default. I added this flag to `-Wparentheses` for gcc compatibility.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142800/new/

https://reviews.llvm.org/D142800