[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Nico Weber via cfe-commits]

On Tue, Apr 10, 2018 at 12:13 PM, David Blaikie <dblaikie at gmail.com> wrote:

> It's more the fact that that's /all/ the warning improvement has found so
> far. If it was 8 false positives & also found 80 actionable bugs/bad code,
> that'd be one thing.
>

Right. We used to hold ourselves to very high standards for warnings. I'd
like us to keep doing that.

Now, admittedly, maybe it would help find bugs that people usually catch
> with a unit test, etc but never make it to checked in code - that's always
> harder to evaluate though Google has some infrastructure for it at least.
>
> On Tue, Apr 10, 2018 at 9:07 AM John McCall <rjmccall at gmail.com> wrote:
>
>> If you have a concrete suggestion of how to suppress this warning for
>> user-defined operators just in unit tests, great. I don’t think 8
>> easily-suppressed warnings is an unacceptably large false-positive problem,
>> though. Most warnings have similar problems, and the standard cannot
>> possibly be “must never fire on code where the programmer is actually happy
>> with the behavior”.
>>
>> John.
>> On Tue, Apr 10, 2018 at 17:12 Nico Weber <thakis at chromium.org> wrote:
>>
>>> "False positive" means "warning fires but didn't find anything
>>> interesting", not "warning fires while being technically correct". So all
>>> these instances do count as false positives.
>>>
>>> clang tries super hard to make sure that every time a warning fires, it
>>> is useful for a dev to fix it. If you build with warnings enabled, that
>>> should be a rewarding experience. Often, this means dialing back a warning
>>> to not warn in cases where it would make sense in theory when in practice
>>> the warning doesn't find much compared to the amount of noise it generates.
>>> This is why for example clang's -Woverloaded-virtual is usable while gcc's
>>> isn't (or wasn't last I checked a while ago) – gcc fires always when it's
>>> technically correct to do so, clang only when it actually matters in
>>> practice.
>>>
>>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via
>>> cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>>
>>>> lebedev.ri added a comment.
>>>>
>>>> In https://reviews.llvm.org/D44883#1063003, @thakis wrote:
>>>>
>>>> > This landing made our clang trunk bots do an evaluation of this
>>>> warning :-P It fired 8 times, all false positives, and all from unit tests
>>>> testing that operator= works for self-assignment. (
>>>> https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has
>>>> the exact details) It looks like the same issue exists in LLVM itself too,
>>>> https://reviews.llvm.org/D45082
>>>>
>>>>
>>>> Right, i guess i only built the chrome binary itself, not the tests,
>>>> good to know...
>>>>
>>>> > Now tests often need warning suppressions for things like this, and
>>>> this in itself doesn't seem horrible. However, this change takes a warning
>>>> that was previously 100% noise-free in practice and makes it pretty noisy –
>>>> without a big benefit in practice. I get that it's beneficial in theory,
>>>> but that's true of many warnings.
>>>> >
>>>> > Based on how this warning does in practice, I think it might be
>>>> better for the static analyzer, which has a lower bar for false positives.
>>>>
>>>> Noisy in the sense that it correctly diagnoses a self-assignment where
>>>> one **intended** to have self-assignment.
>>>> And unsurprisingly, it happened in the unit-tests, as was expected ^ in
>>>> previous comments.
>>>> **So far** there are no truly false-positives noise (at least no
>>>> reports of it).
>>>>
>>>> We could help workaround that the way i initially suggested, by keeping
>>>> this new part of the diag under it's own sub-flag,
>>>> and suggest to disable it for tests. But yes, that
>>>>
>>>
>>>>
>>>> Repository:
>>>>   rC Clang
>>>>
>>>> https://reviews.llvm.org/D44883
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180410/eedeefa2/attachment-0001.html>