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

[David Blaikie via Phabricator via libcxx-commits]

dblaikie added a comment.

In D147844#4329497 <https://reviews.llvm.org/D147844#4329497>, @aaron.ballman wrote:

> In general, I think this is incremental progress on the diagnostic behavior. However, it's clear that there is room for interpretation on what is or is not a false positive diagnostic for this,

I hope we can agree on what a false positive is here - when the warning fires but the code is what the developer intended (ie: the existing code with the existing language semantics produce the desired result, the "fix" is to add parentheses that explicitly encode the language's existing rules/behavior anyway).

Not that we don't have warnings that do this - that encourage parens to reinforce what the language already does to be more explicit/intentional about it, and in some cases it's not that uncommon that the user will be adding parens that reinforce the precedence rules anyway.

Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of them found bugs/unintended behavior) Are there any examples of bugs being found by this warning in a codebase? (& how many false positives in such a codebase did it also flag?)

> so we should pay close attention to user feedback during the 17.x release cycle. If it seems we're getting significant push back, we may need to come back and rethink.
>
> Specifically, I think we've improved the situation for code like this:
>
>   // `p` is a pointer, `x` is an `int`, and `b` is a `bool`
>   p + x ? 1 : 2; // previously didn't warn, now warns
>   p + b ? 1 : 2; // always warned
>
> Does anyone feel we should not move forward with accepting the patch in its current form?

*goes to look*

Mixed feelings - you're right, incremental improvement/adding missed cases to an existing warning (especially if that warning's already stylistic in nature) is a lower bar/doesn't necessarily need the false positive assessment. But it looks like this case might've been intentionally suppressed in the initial warning implementation? (anyone linked to the original warning implementation/review/design to check if this was intentional?)

But, yeah, seems marginal enough I don't have strong feelings either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147844