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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
