aaron.ballman added a comment. 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! >> 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
