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

[David Blaikie via cfe-commits]

On Mon, Apr 2, 2018 at 8:05 AM Roman Lebedev via Phabricator <
reviews at reviews.llvm.org> wrote:

> lebedev.ri added a comment.
>
> In https://reviews.llvm.org/D44883#1054326, @thakis wrote:
>
> > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote:
> >
> > > Historically Clang's policy on warnings was, I think, much more
> > >  conservative than it seems to be today. There was a strong desire not
> to
> > >  implement off-by-default warnings, and to have warnings with an
> > >  exceptionally low false-positive rate - maybe the user-defined
> operator
> > >  detection was either assumed to, or demonstrated to, have a
> sufficiently
> > >  high false positive rate to not meet that high bar.
> >
> >
> > This is still the case. For a new warning, you should evaluate some
> large open-source codebase and measure true positive and false positive
> rate and post the numbers here.
>
>
> I did run it on stage-2 of LLVM itself. The only 'false-positives' so far
> are already present in trunk:
>

To clarify a little - "false positive" in the sense Nico (& I) are using it
is "any case in which the warning fired on code that was already working as
intended" (ie: where the warning didn't represent a real bug - or at least
represent a confusing/surprising thing that would be worth/benefit from
rewriting to some other less confusing form).

So the cases you found in the tests for certain self assignment would also
be false positives.

Not necessarily enough to warrant/argue against moving forward on this
warning, but they are imho, false positives for the purpose of discussions
like this.


> https://godbolt.org/g/SXc4Wd
>
>   void test_int () {
>       int b;
>       static_assert(noexcept(b &= b), "" );
>   }
>   void test_byte () {
>       std::byte b;
>       static_assert(noexcept(b &= (std::byte &)b), "" );
>   }
>
>   <source>:4:30: warning: explicitly assigning value of variable of type
> 'int' to itself [-Wself-assign]
>
>       static_assert(noexcept(b &= b), "" );
>
>                              ~ ^  ~
>
> Should it really warn when in unevaluated context?
> Dunno, but trunk already does that. Should that be changed?
>
> Which other large codebase do you want me to evaluate, so we can talk
> facts?
>
> >> (as for the flag splitting - that was sometimes done if the new variant
> of
> >>  a flag had enough bug-finding power that an existing codebase using the
> >>  existing flag behavior would need significant cleanup - by having the
> new
> >>  functionality under another flag name, existing codebases upgrading to
> a
> >>  newer compiler wouldn't be forced to either do all that cleanup
> up-front or
> >>  disable the flag & risk regressions... )
>
>
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D44883
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180402/75ea917f/attachment-0001.html>