aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14134
return;
-
+
Sema.Diag(Loc, diag::warn_identity_field_assign) << 0;
----------------
Spurious whitespace change.
================
Comment at: clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp:71-91
+ this->a *= a;
+ this->a /= a; // expected-warning {{assigning field to itself}}
+ this->a %= a; // expected-warning {{assigning field to itself}}
+ this->a += a;
+ this->a -= a; // expected-warning {{assigning field to itself}}
+ this->a <<= a;
+ this->a >>= a;
----------------
The behavior of this diagnostic is almost inscrutable without really sitting
down to think about it... and I'm not even certain I get the logic for it
despite thinking about it heavily for a while now. I think my confusion boils
down to this diagnostic trying to diagnose two different kinds of problems.
Situation 1 is where there's a possible typo and the user meant a different
object:
```
a /= a;
this->a /= this->a;
// Less-contrived example
a00 /= a01;
a01 /= a02;
a02 /= a02; // Oops, typo!
```
(Note, I think `this->a` and `a` should be handled the same in this case
because there are coding styles out there that mandate adding `this->` to any
member lookup, so there's no reason to think that `this->a /= this->a;` is any
less of a typo than `a /= a;`.)
Situation 2 is where the user is trying to work around the user-unfriendly
lookup rules of C++, or there is a typo, but got confused because lookup found
the same object on both sides:
```
this->a /= a;
a /= this->a;
// Less-contrived example
int a;
struct S {
int a;
void foo() {
// Divide the member variable by the global variable
this->a /= a; // Oops, that's not how this works in practice
}
void bar(int ab) {
// Divide the member variable by the parameter
this->a /= a; // Oops, typo, meant to use 'ab' instead
}
};
```
So I'm surprised that we'd want to silence the warnings on `a /= a;` as we're
doing here. Frankly, I think we should be silent on `a *= a;` because that's an
extremely common way to square a variable, but diagnose almost all the other
cases with a "did you mean?" diagnostic. `a -= a;` and `a %= a;` are bad ways
to write `a = 0`, `a += a;` is a bad way to write `a *= 2;` (but might be
common enough we want to silence it as well), and `a /= a;` is a bad way to
write `a = 1;` (The bitwise operators may still be reasonable to silence the
warning on, but even `a &= a;` and `a |= a;` seems like weird no-ops...)
All that said, my understanding of the crux of #42469 is that we wanted to
silence the warning whenever the operator being used is overloaded because we
have no idea whether self-assign is intended in that case. So I would expect
these cases to all consistently not diagnose because they're all using the
overloaded operator.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146897/new/
https://reviews.llvm.org/D146897
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
