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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
