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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: thakis.
lebedev.ri added a comment.

In https://reviews.llvm.org/D44883#1048421, @Quuxplusone wrote:

> Two more high-level comments from me, and then I'll try to butt out.
>
> Q1. I don't understand what `field-` is doing in the name of some diagnostics. Is there a situation where `x = x;` is "less/more of an error" just because `x` is a {data member, lambda capture, structured binding} versus not? Why should the "plain old variable" case be separately toggleable? (This question pre-dates Roman's proposed patch AFAICT. I think the answer is that self-assignment specifically //of a data member// specifically //in a constructor// is considered "more of an error" because it's likely to be a very particular kind of typo-bug. I doubt this answer is quite strong enough to justify multiple warning options, though.)


I think we should ask @thakis that - https://reviews.llvm.org/rL159394

> Q2. I don't understand why `x &= x` (and now `x /= x`, thanks) should be treated as "more of an error" than `x & x` (and now `x / x`), or for that matter `x == x`. The sin here is not "self-assignment" per se; it's "over-complicating a tautological mathematical operation." I admit I haven't thought about it as much as you folks, but the more I think about it, the more I think the only consistent thing for Clang to do would be to back off from warning about anything beyond plain old `x = x`, and leave *all* the mathematical-tautology cases to checkers like clang-tidy and PVS-Studio.

(oh please no pvs-studio stuff here too, it's getting a 'bit' spammy lately)

In https://reviews.llvm.org/D44883#1048400, @rjmccall wrote:

> 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?


I'm not saying that is not a valid concern. I'm simply following the pre-existing practice, which is, as far i'm aware, to split the diag groups if it makes sense.

> Because I do think we should warn on field self-assignments for user-defined operators.

I agree, as i said, i did not want to put everything into one bit differential.

> 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.

Can you elaborate a bit more, given this struct, which of these variants should be diagnosed as user-provided, and which as builtin?

  struct S {}; // certainly builtin?

  struct S {
    S() = default; // builtin?
    S &operator=(const S &) = default; // builtin?
    S &operator=(S &) = default;  // builtin?
  
    S &operator=(const S &); // certainly user-provided?
    S &operator=(S &); // certainly user-provided?
  };



>> 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.

+1


Repository:
  rC Clang

https://reviews.llvm.org/D44883