[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:12093
+    break;
+  }
+
----------------
lebedev.ri wrote:
> rjmccall wrote:
> > lebedev.ri wrote:
> > > rjmccall wrote:
> > > > lebedev.ri wrote:
> > > > > rjmccall wrote:
> > > > > > I think doing this here can result in double-warning if the overload resolves to a builtin operator.  Now, it might not actually be possible for that to combine with the requirements for self-assignment, but still, I think the right place to diagnose this for C++ is the same place we call DiagnoseSelfMove in CreateOverloadedBinOp.
> > > > > > 
> > > > > > Can CheckIdentityFieldAssignment just be integrated with DiagnoseSelfAssignment so that callers don't need to do call both?
> > > > > > I think the right place to diagnose this for C++ is the same place we call DiagnoseSelfMove in CreateOverloadedBinOp.
> > > > > 
> > > > > ```
> > > > >         switch (Opc) {
> > > > >         case BO_Assign:
> > > > >         case BO_DivAssign:
> > > > >         case BO_SubAssign:
> > > > >         case BO_AndAssign:
> > > > >         case BO_OrAssign:
> > > > >         case BO_XorAssign:
> > > > >           DiagnoseSelfAssignment(Args[0], Args[1], OpLoc);
> > > > >           CheckIdentityFieldAssignment(Args[0], Args[1], OpLoc);
> > > > >           break;
> > > > >         default:
> > > > >           break;
> > > > >         }
> > > > > 
> > > > >         // Check for a self move.
> > > > >         if (Op == OO_Equal)
> > > > >           DiagnoseSelfMove(Args[0], Args[1], OpLoc);
> > > > > ```
> > > > > 
> > > > > 
> > > > > ^ That does not appear to work. Pretty much all these tests start to fail.
> > > > > 
> > > > Okay.  It's possible that my suggestion is wrong.  Can you explain more how they fail?
> > > Right, i should have been verbose :)
> > > There are no test changes as compared to the current diff.
> > > Here is the output of `$ ninja check-clang-sema check-clang-semacxx`
> > > {F5920055}
> > > It is also totally possible that i'm missing something obvious on my end...
> > Oh, DiagnoseSelfAssignment disables itself during template instantiation, presumably because it's an easy syntactic check that will always warn on the parsed code and so doesn't need to warn again during instantiation.
> > 
> > In that case, I think the place you had the check is fine.
> Am i correctly reading that as "ok, keep it as it is, in `BuildOverloadedBinOp()`" ?
Yes, I think that's the right place for it, given that it's basically designed to only fire during parsing.

We could also just move the check (in all cases) to ActOnBinOp, which is not called by template instantiation.


Repository:
  rC Clang

https://reviews.llvm.org/D44883