[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#1048439, @lebedev.ri wrote:

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


It's fine to warn about really obvious tautologies in the compiler.  We don't need to go much further than this, though.

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

I agree.  In general, I think this would be fine; my only concern is because we do already have some splitting along a different dimension, so we do need to stop and think about it.  Maybe the answer is that

>> 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 don't think it would over-complicate this patch to handle all the cases at once.

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

That's an interesting question!  You're absolutely right, I do think we should warn about trivial C++ assignments as part of the builtin group.  If a C program includes a self-assignment that happens to be of struct type, we'll warn about that, and it seems to be that we shouldn't lose the warning just because that code is compiled as C++ instead.

As for your example, I think it should be based on whether the assignment operator selected is trivial.  If it's non-trivial, even if it's defaulted, we should warn about it as a "user-defined" operator, since ultimately it does involve calling such an operator.


Repository:
  rC Clang

https://reviews.llvm.org/D44883