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