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