[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 7:21 AM Roman Lebedev via Phabricator <
reviews at reviews.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).
>

False positive in the broad sense (which I think I mentioned in review of
this patch) of "diagnosing code that is working as intended (rather than
diagnosing/identifying bugs/mistakes) & is fairly unsurprising/legible (ie:
the warning didn't find places that weren't bugs, but were sufficiently
confusing that they could be written better some other way)".

In the past, the bar for warnings has been higher in my experience (though
an interesting argument about whether making a change to an existing
warning, versus adding a warning under a separate flag, changes the
decision (since it may wash out new false negatives with the many true
negatives in the original implementation)).

Probably one of the best ways to evaluate whether this is beneficial in
practice is to leave it in (at least inside Google or anywhere else that
can track diagnostic frequency) for a while & see how often it comes up in
practice... - though I don't know if we'd be able to separate out the cases
that come from this improvement versus all the rest - maybe if the text is
(or was made to be) worded slightly differently, or if it were under a
separate flag.

(but yes, I recall the discussion about flag naming and subsetting the
flags, etc - I don't have a good answer to that)


> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180410/589c07d0/attachment.html>