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