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

Thu Mar 2 10:23:10 PST 2023

aaron.ballman added a comment.

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.

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

https://reviews.llvm.org/D142800