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

[David Blaikie via cfe-commits]

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/18566b85/attachment.html>