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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D147844#4293988 <https://reviews.llvm.org/D147844#4293988>, @dblaikie wrote:

> In D147844#4293743 <https://reviews.llvm.org/D147844#4293743>, @cjdb wrote:
>
>> In D147844#4293693 <https://reviews.llvm.org/D147844#4293693>, @dblaikie wrote:
>>
>>>> I think some of the cases are ambiguous while others are not.
>>>
>>> Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant frequency in a codebase?)
>>
>> I disagree that there's a need for bugs to add this warning. Reading ambiguous-but-correct code isn't going to be buggy, but it is going to cause readability issues for any reviewers or maintainers.
>
> That's generally been the bar we use for evaluating warnings - does it find bugs. Especially because if it doesn't, it's unlikely to be turned on on large pre-existing codebases owing to the cost of cleaning them up with limited value in terms of improving readability but not finding any bugs. (& goes hand-in-hand with the general policy of not adding off-by-default warnings, because they don't get used much and come at a cost to clang's codebase (& some (death-by-a-thousand-cuts) cost to compile time performance, even when the warning is disabled))
>
> Readability improvements that don't cross the threshold to be the cause of a significant number of bugs are moreso the domain of clang-tidy, not clang warnings.

Fair enough, I often mix up which project a warning belongs to, so thanks for the reminder :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147844