[libcxx-commits] [PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman added reviewers: dblaikie, cjdb, echristo, clang-language-wg.
aaron.ballman added a comment.

In D147844#4286500 <https://reviews.llvm.org/D147844#4286500>, @philnik wrote:

> I have to say I'm not really convinced this change is a good idea. The cases it flags don't really seem in any way ambiguous/erroneous.

I think some of the cases are ambiguous while others are not. It's taken me a while to come up with what I think my brain is doing, maybe this happens for others as well. When the ternary conditional expression is a binary expression, my brain can figure things out very quickly when the RHS of that binary expression is a literal but my brain is much less confident when the RHS of that binary expression is an identifier. e.g., `x & 1 ? foo : bar` is easy for my brain to go "well, obviously the condition is not testing whether `1` is nonzero, so it must be testing the result of `x & 1` instead", but something like `x & y ? foo : bar` is far less obvious as to what the behavior is because `x & (y ? foo : bar)` is as reasonable a choice as `(x & y) ? foo : bar` without putting a lot more thought into it. However, that might be splitting hairs with the diagnostic functionality (esp because macros and enumerations are sort of like literals and sort of like identifiers, depending on the way you want to think of them).

Adding in some more folks for opinions on whether the proposed changes in this patch are an improvement or whether we want to tweak the heuristic a bit more. My original thinking was that all precedence situations with ternary conditional expressions are kind of equally confusing, but given that two folks have pushed back on the proposed changes, more opinions would be valuable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147844