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

Tue Apr 10 09:36:09 PDT 2018

Any good ideas on how to evaluate it, then? (in addition to/other than
something like Google where we can track the diagnostic for - a few months?)

On Tue, Apr 10, 2018 at 9:34 AM John McCall <rjmccall at gmail.com> wrote:

> Yeah, -Wself-assign in general is about catching a class of live-coding
> errors which is pretty unlikely to ever make it into committed code, since
> the code would need to be dead/redundant for the mistake to not be noticed.
> So it’s specifically a rather poor match for the static analyzer, and I
> would not expect it to catch much of anything on production code.
>
> John.
> On Tue, Apr 10, 2018 at 18:27 David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev <lebedev.ri at gmail.com>
>> wrote:
>>
>>> Because *so far* it has been running on the existing code, which i guess
>>> was already pretty warning free, and was run through the static
>>> analyzer,
>>
>> which obviously should catch such issues.
>>>
>>
>> Existing code this has been run over isn't necessarily run against the
>> static analyzer already. (this has been run against a subset of (maybe all
>> of, I forget) google, which isn't static analyzer clean by any stretch (I'm
>> not even sure if the LLVM codebase itself is static analyzer clean)).
>>
>>
>>> Do you always use [clang] static analyzer, each time you build?
>>> Great! It is indeed very good to do so. But does everyone?
>>>
>>> This particular issue is easily detectable without full static analysis,
>>> and as it is seen from the differential, was very straight-forward to
>>> implement
>>> as a simple extension of pre-existing -Wself-assign.
>>>
>>> So if this is really truly unacceptable false-positive rate, ok, sure,
>>> file a RC bug,
>>> and i will split it so that it can be simply disabled for *tests*.
>>>
>>> That being said, i understand the reasons behind "keep clang diagnostics
>>> false-positive free". I don't really want to change that.
>>>
>>> On Tue, Apr 10, 2018 at 7: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.
>>> >
>>> > 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/2558142e/attachment-0001.html>