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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
