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