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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D44883#1048081, @lebedev.ri wrote:

> In https://reviews.llvm.org/D44883#1048010, @rjmccall wrote:
>
> > I'm not sure you really need to put these in their own warning sub-group just because they're user-defined operators.  That's especially true because it appears we already have divisions in the warning group based on the form of the l-value; we don't want this to go combinatorial.
>
>
> Several reasons:
>
> - The initial `-Wself-assign` was intentionally implemented not to warn on overloaded operators. https://github.com/llvm-mirror/clang/commit/9f7a6eeee441bcbb1b17208cb3abd65a0017525a#diff-e0deb7b32f28507a3044a6bf9a63b515R31 (https://reviews.llvm.org/rL122804)


That's interesting.  Maybe Chandler remembers the rationale for that?

> - While it is an obvious bug when self-operation happens with builtin operators, i'm less certain of that with overloaded operators.

It's certainly more plausible that the programmer has a good reason to self-assign with a user-defined operator, yeah.

>   If you happen to be routinely using self-assignment via oh-so-very-special overloaded operator=, and you don't like to have this
>   diagnostic, you could just disable it, and not loose the coverage of the `-Wself-assign-builtin`.
>   If it is all in one group, you can't do that...

True.

> - Based on previous expirience, separate diag groups are good, see e.g https://reviews.llvm.org/D37620, https://reviews.llvm.org/D37629
> - I'm failing to find the original quote, but i **think** @rsmith said something along the "diag groups are cheap, use them". But i may as well be mis-remembering/having false memories here, sorry.

Yeah, like I said, I'm just worried about the usability of having multiple dimensions of warning group here.  Like, do we need both -Wself-assign-field-builtin and -Wself-assign-field-overloaded?  Because I do think we should warn on field self-assignments for user-defined operators.

I think the term ought to be "user-defined" rather than "overloaded", by the way.  In a sense, these operators aren't really any more overloaded than the builtin operators (at least, not necessarily so) — C++ even formalizes the builtin operators as being a large family of overloads.  And the reason we want to treat them specially is that they have user-provided definitions that might be doing special things, not because of anything to do with name resolution.

> TLDR: if you insist, sure, i can just cram it into the already-existing `-Wself-assign`,
>  but i believe that is the opposite of what should be done, and is against the way it was done previously.

I'm not going to insist.  But I would like to hear if Chandler remembers why his patch didn't warn about user-defined operators.


Repository:
  rC Clang

https://reviews.llvm.org/D44883