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