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

[NagaChaitanya Vellanki via Phabricator via libcxx-commits]

chaitanyav added a comment.

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

> In D147844#4335598 <https://reviews.llvm.org/D147844#4335598>, @dblaikie wrote:
>
>> 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).
>
> I agree with that definition -- that's a useful way to approach this, thank you!
>
>> 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.
>
> Yup, which is largely what this patch is about.
>
>> Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of them found bugs/unintended behavior)
>
> Yes, they all are false positives by the above definition.
>
>> 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?)
>
> This would be good to know, but a bit of a heavy lift to require of @chaitanyav because they were working on this issue (https://github.com/llvm/llvm-project/issues/61943) with a "good first issue" label that is starting to look a bit like it was misleading (sorry about that!). However, if you're able to try compiling some larger projects with your patch applied to see if it spots any bugs in real world code, that would be very helpful!

@aaron.ballman will try this patch on some projects and post the results here

>>> 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?)
>
> I tried to chase down where this got added to see what review comments there were, but it seems to predate my involvement (I'm seeing mentions of this in 2011).
>
>> But, yeah, seems marginal enough I don't have strong feelings either way.
>
> Thank you for the opinion! I think pointer + int is a far more common code pattern than pointer + bool, so it makes some sense to me that we would silence the first case while diagnosing the second case. Given the general lack of enthusiasm for the new diagnostics, it may boil down to answering whether this finds any true positives or not.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147844