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

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 511795.
python3kgae added a comment.

Update the comment done by rjmccall.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-self-assign-overloaded.cpp


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15657,13 +15657,22 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// In the non-overloaded case, we warn about self-assignment (x = x) for
+// both simple assignment and certain compound assignments where algebra
+// tells us the operation yields a constant result.  When the operator is
+// overloaded, we can't do the latter because we don't want to assume that
+// those algebraic identities still apply; for example, a path-building
+// library might use operator/= to append paths.  But it's still reasonable
+// to assume that simple assignment is just moving/copying values around
+// and so self-assignment is likely a bug.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15657,13 +15657,22 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// In the non-overloaded case, we warn about self-assignment (x = x) for
+// both simple assignment and certain compound assignments where algebra
+// tells us the operation yields a constant result.  When the operator is
+// overloaded, we can't do the latter because we don't want to assume that
+// those algebraic identities still apply; for example, a path-building
+// library might use operator/= to append paths.  But it's still reasonable
+// to assume that simple assignment is just moving/copying values around
+// and so self-assignment is likely a bug.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15660
   case BO_Assign:
+// Skip diagnose on compound assignment.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);




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

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

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 511697.
python3kgae added a comment.

Revert change for self-assign-field which deserve its own pull request.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-self-assign-overloaded.cpp


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15657,13 +15657,15 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// Skip diagnose on compound assignment.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15657,13 +15657,15 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// Skip diagnose on compound assignment.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Totally fair :)


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D146897#4249300 , @rjmccall wrote:

> The third idea seems like a valuable warning, but it's basically a 
> *different* warning and shouldn't be lumped into this one.  I understand the 
> instinct to carve it out here so that we don't regress on warning about it, 
> but I think we should just do it separately.  And we should do it much more 
> generally, because there's no reason that logic is limited to just these 
> specific compound operators, or in fact to any individual operator; we should 
> probably warn about all inconsistent references to the same variable within a 
> single statement.

Yeah, good point; I'm convinced.

> (I'd draw the line at statement boundaries, though, because requiring 
> function-wide consistency could be really noisy.)

At least, I don't think we should warn if both `a` and `this->a` are used in 
different statements in the same function, and `a` is shadowed by a local 
declaration wherever `this->a` is used, and I think it might be reasonable to 
not warn on a member `a` if the name `a` is ever declared in the function 
regardless of where it's in scope. For the remaining cases, where `a` is never 
declared locally, I suspect the false positive rate for warning if the function 
uses both `a` and `this->a` would be lower, but we could probably get some data 
on that pretty easily. Speaking of data, it'd be nice to get some evidence that 
this is a mistake that actually happens before landing a dedicated warning for 
it :)


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> we should probably warn about all inconsistent references to the same 
> variable within a single statement.

+1 here, that that seems like the suitable generalization/unrelated to the use 
in/restriction to assignments


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

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, I feel there are three ideas here:

- Clang should warn about self simple assignment in all cases, because it's 
probably a mistake.  We can assume it's a mistake because it's reasonable to 
assume that the simple assignment operator behaves like a value assignment, 
even if it's user-defined, and overwriting a value with itself is a pointless 
operation.
- Clang should warn about self compound assignment when the type is arithmetic 
for these specific operators where algebraically `x OP x` would yield a 
constant value, because it's probably a mistake.  This is because we know the 
exact behavior of the operator, and producing a constant value by cancellation 
is a pointless operation.
- Clang should warn about inconsistent use of `this` on self compound 
assignment for all operators, because it's probably a mistake.  This is because 
the implication of writing the member reference two different ways is that 
there are two different variables in play, but in fact there are not.

The third idea seems like a valuable warning, but it's basically a *different* 
warning and shouldn't be lumped into this one.  I understand the instinct to 
carve it out here so that we don't regress on warning about it, but I think we 
should just do it separately.  And we should do it much more generally, because 
there's no reason that logic is limited to just these specific compound 
operators, or in fact to any individual operator; we should probably warn about 
all inconsistent references to the same variable within a single statement. 
(I'd draw the line at statement boundaries, though, because requiring 
function-wide consistency could be really noisy.)


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 510976.
python3kgae added a comment.

skip warning on field for compound assignment when isImplicitCXXThis matches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146897/new/

https://reviews.llvm.org/D146897

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp
  clang/test/SemaCXX/warn-self-assign-overloaded.cpp

Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp
@@ -58,15 +58,49 @@
 
 #ifndef DUMMY
 a *= a;
-a /= a; // expected-warning {{assigning field to itself}}
-a %= a; // expected-warning {{assigning field to itself}}
+a /= a;
+a %= a;
 a += a;
-a -= a; // expected-warning {{assigning field to itself}}
+a -= a;
 a <<= a;
 a >>= a;
-a &= a; // expected-warning {{assigning field to itself}}
-a |= a; // expected-warning {{assigning field to itself}}
-a ^= a; // expected-warning {{assigning field to itself}}
+a &= a;
+a |= a;
+a ^= a;
+
+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;
+this->a &= a; // expected-warning {{assigning field to itself}}
+this->a |= a; // expected-warning {{assigning field to itself}}
+this->a ^= a; // expected-warning {{assigning field to itself}}
+
+a *= this->a;
+a /= this->a; // expected-warning {{assigning field to itself}}
+a %= this->a; // expected-warning {{assigning field to itself}}
+a += this->a;
+a -= this->a; // expected-warning {{assigning field to itself}}
+a <<= this->a;
+a >>= this->a;
+a &= this->a; // expected-warning {{assigning field to itself}}
+a |= this->a; // expected-warning {{assigning field to itself}}
+a ^= this->a; // expected-warning {{assigning field to itself}}
+
+this->a *= this->a;
+this->a /= this->a;
+this->a %= this->a;
+this->a += this->a;
+this->a -= this->a;
+this->a <<= this->a;
+this->a >>= this->a;
+this->a &= this->a;
+this->a |= this->a;
+this->a ^= this->a;
+
 #endif
   }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14099,7 +14099,7 @@
 }
 
 static void CheckIdentityFieldAssignment(Expr *LHSExpr, Expr *RHSExpr,
- SourceLocation Loc,
+ bool IsCompound, SourceLocation Loc,
  Sema ) {
   if (Sema.inTemplateInstantiation())
 return;
@@ -14116,6 +14116,10 @@
   if (ML && MR) {
 if (!(isa(ML->getBase()) && isa(MR->getBase(
   return;
+// For compound case, only warning on this->x -= x or x -= this->x.
+if (IsCompound && ML->getBase()->isImplicitCXXThis() ==
+  MR->getBase()->isImplicitCXXThis())
+  return;
 const ValueDecl *LHSDecl =
 cast(ML->getMemberDecl()->getCanonicalDecl());
 const ValueDecl *RHSDecl =
@@ -14127,7 +14131,7 @@
 if (const ReferenceType *RefTy = LHSDecl->getType()->getAs())
   if (RefTy->getPointeeType().isVolatileQualified())
 return;
-
+
 Sema.Diag(Loc, diag::warn_identity_field_assign) << 0;
   }
 
@@ -14171,7 +14175,8 @@
   if (CompoundType.isNull()) {
 Expr *RHSCheck = RHS.get();
 
-CheckIdentityFieldAssignment(LHSExpr, RHSCheck, Loc, *this);
+CheckIdentityFieldAssignment(LHSExpr, RHSCheck, /*IsCompound*/ false, Loc,
+ *this);
 
 QualType LHSTy(LHSType);
 ConvTy = CheckSingleAssignmentConstraints(LHSTy, RHS);
@@ -15644,14 +15649,17 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// Skip diagnose on compound assignment.
+

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15656
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;

python3kgae wrote:
> rsmith wrote:
> > This is the same thing, but for `this->x += this->x`. I think we should 
> > also suppress those warnings for compound assignment, except when one of 
> > the operands is an implicit member access and one is an explicit member 
> > access (`this->x += x;` should still warn if the latter `x` is also 
> > interpreted as `this->x`).
> For case like this:
> 
> ```
> class Vector {
> public:
> Vector& operator+=(const Vector ) { return *this; }
> Vector& operator-=(const Vector ) { return *this; }
> };
> 
> class A {
> public:
>   A(){}
>   Vector x;
>   void foo() {
>  this->x -= this->x;
>  this->x -= x;
>  this->x += this->x;
>  this->x += x;
>   }
> };
> ```
> clang will report 2 warning:
> 
> ```
> :14:14: warning: assigning field to itself [-Wself-assign-field]
>  this->x -= this->x;
>  ^
> :15:14: warning: assigning field to itself [-Wself-assign-field]
>  this->x -= x;
> ```
> 
> Do you mean we should make it report warning on this->x -= x; and this->x += 
> x; ?
Let's not add any new warnings. Pretend my comment was about `-=`, not `+=`.


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

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

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15656
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;

rsmith wrote:
> This is the same thing, but for `this->x += this->x`. I think we should also 
> suppress those warnings for compound assignment, except when one of the 
> operands is an implicit member access and one is an explicit member access 
> (`this->x += x;` should still warn if the latter `x` is also interpreted as 
> `this->x`).
For case like this:

```
class Vector {
public:
Vector& operator+=(const Vector ) { return *this; }
Vector& operator-=(const Vector ) { return *this; }
};

class A {
public:
  A(){}
  Vector x;
  void foo() {
 this->x -= this->x;
 this->x -= x;
 this->x += this->x;
 this->x += x;
  }
};
```
clang will report 2 warning:

```
:14:14: warning: assigning field to itself [-Wself-assign-field]
 this->x -= this->x;
 ^
:15:14: warning: assigning field to itself [-Wself-assign-field]
 this->x -= x;
```

Do you mean we should make it report warning on this->x -= x; and this->x += x; 
?


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15656
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;

This is the same thing, but for `this->x += this->x`. I think we should also 
suppress those warnings for compound assignment, except when one of the 
operands is an implicit member access and one is an explicit member access 
(`this->x += x;` should still warn if the latter `x` is also interpreted as 
`this->x`).


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

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

[Xiang Li via Phabricator via cfe-commits]

python3kgae created this revision.
python3kgae added reviewers: rjmccall, Quuxplusone, riccibruno.
Herald added a project: All.
python3kgae requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes 42469 https://github.com/llvm/llvm-project/issues/42469

Only check self assignment on BO_Assign when BuildOverloadedBinOp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146897

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-self-assign-overloaded.cpp


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15644,13 +15644,15 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// Skip diagnose on compound assignment.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:


Index: clang/test/SemaCXX/warn-self-assign-overloaded.cpp
===
--- clang/test/SemaCXX/warn-self-assign-overloaded.cpp
+++ clang/test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -53,15 +53,15 @@
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15644,13 +15644,15 @@
Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+// Skip diagnose on compound assignment.
+DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+[[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
 CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
 break;
   default:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits