[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

[Aaron Ballman via Phabricator via cfe-commits]

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