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

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

> 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.
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.

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.

tensorflow/tensorflow newly-warned lines:

- https://github.com/tensorflow/tensorflow/blob/12eef948f12e47ab9ce736d8967c72fc4436db79/tensorflow/compiler/xla/overflow_util.h#L67
- https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L366
- https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L436
- https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L457
- https://github.com/tensorflow/tensorflow/blob/9edf0d9eb621f18217c2a0cc5d9b67f53b40901e/tensorflow/compiler/xla/service/while_loop_analysis.cc#L364
- https://github.com/tensorflow/tensorflow/blob/9edf0d9eb621f18217c2a0cc5d9b67f53b40901e/tensorflow/compiler/xla/service/while_loop_analysis.cc#L377


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

https://reviews.llvm.org/D142800