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

Tue Apr 10 09:34:01 PDT 2018

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/llvm-commits/attachments/20180410/2f1ccc86/attachment.html>