Rebase and Ping

Address comments from @shafik and @tbaeder

Address comments from @aaron.ballman

• NFC stylistic changes

Address comments from @aaron.ballman

• Dropped const qualifier from value objects for style consistency
• Passed boolean HasFallThroughAttr to callback and handled warning suppression in HandleUnreachable in order to make code cleaner.

In D145793#4189252, @aaron.ballman wrote:

LGTM, though please add a release note for the fix. If you need someone to land on your behalf, let us know what name and email address you'd like used for patch attribution.

Added a release note

Address comments from @shafik

• Pass bool flag UnreachableFallThroughDiagIsEnabled instead of DiagnosticsEngine

Address comment from @aaron.ballman

• Rewording the diagnostic

In D142800#4165090, @aaron.ballman wrote:

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

Update the differential

• Revise warning messages based on ideas from @aaron.ballman.
• Introduce a new warning flag -Wchaining-comparisons that is enabled by default, to warn about chaining relationals or equal operators.
• Adjust existing test files to conform with the new warning settings.

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.

Address comments from erichkeane:

• Fix comment wording
• Avoid using macro in test file

Fix the new warning issued against libcxx test code.

I added the release note.

Also, do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

I don't have commit access. Would you please land this patch for me? Please use "Takuya Shimizu <shimizu2486@gmail.com>" for the patch attribution.
Thank you.

In D140860#4047534, @aaron.ballman wrote:

In D140860#4045224, @dblaikie wrote:

In D140860#4044937, @aaron.ballman wrote:

In D140860#4031872, @dblaikie wrote:

The risk now is that this might significantly regress/add new findings for this warning that may not be sufficiently bug-finding to be worth immediate cleanup, causing users to have to choose between extensive lower-value cleanup and disabling the warning entirely.

Have you/could you run this over a significant codebase to see what sort of new findings the modified warning finds, to see if they're high quality bug finding, or mostly noise/check for whether this starts to detect certain idioms we want to handle differently?

It might be hard to find a candidate codebase that isn't already warning-clean with GCC (at least Clang/LLVM wouldn't be a good candidate because of this) & maybe that's sufficient justification to not worry too much about this outcome...

@aaron.ballman curious what your take on this might be

Thank you for the ping (and the patience waiting on my response)!

I think there's a design here that could make sense to me.

Issuing the diagnostic when there is a literal is silly because the literal value is never going to change. However, with a constant expression, the value could change depending on configuration. This begs the question of: what do we do with literals that are expanded from a macro? It looks like we elide the diagnostic in that case, but macros also imply potential configurability. So I think the design that would make sense to me is to treat macro expansions and constant expressions the same way (diagnose) and only elide the diagnostic when there's a (possibly string) literal. WDYT?

Yeah, I'm OK with that - though I also wouldn't feel strongly about ensuring we warn on the macro case too - if the incremental improvement to do constexpr values is enough for now and a note is left to let someone know they could expand it to handle macros.

But equally it's probably not super difficult to check if the literal is from a macro source location that differs from the source location of either of the operators, I guess? (I guess that check would be needed, so it doesn't warn when the macro is literally 'x && y || true' or the like.

I mostly don't want to insist on dealing with macros in this patch, but it does leave the diagnostic behavior somewhat inconsistent to my mind. I think I can live without the macro functionality though, as this is still forward progress. And yes, you'd need to check the macro location against the operator location, I believe. Testing for a macro expansion is done with SourceLocation::isMacroID(), in case @hazohelet wants to try to implement that functionality as well.

FWIW a lot of build systems support setting CXXFLAGS/CFLAGS before invoking the build system/build generator (cmake, for instance) and respects those - so might be relatively easy to add a new warning flag to the build (& CXX/CC to set the compiler to point to your local build with the patch/changes)

Thanks for your advice! I'll give it a try. It might be a nice opportunity for me to learn some compiler development methods.

I have yet to do thorough checks using this patched clang to build significant code bases.
It will likely take quite a bit of time as I am not used to editing build tool files.

So, so long as a string literal still does the suppression, it's probably fine.

I agree with you. I now also think that integer literals _should not_ do the warning suppression because programmers are sometimes unsure of the expansion result of predefined macros in the compiled environment.
I updated the differential. I am new to Phabricator and made some unnecessary operations. Sorry for bothering you.

add up the former 2 commits into 1

This update limits the warning suppression case to string literals only, and delete no longer necessary functions.

As you point out, enhancement may be more accurate than bug fix.
There are rare cases where enabling a warning for missing parentheses in constexpr logical expressions can be helpful, I think. For example, consider the following code:

constexpr A = ...;
constexpr B = ...;
constexpr C = ...;