lebedev.ri added a comment.
Will do stage2 testing next..
================
Comment at: lib/Sema/SemaExpr.cpp:12087
+ case BO_AndAssign:
+ case BO_OrAssign:
+ DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, /*IsBuiltin=*/false);
----------------
lebedev.ri wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > I understand why `x &= x` and `x |= x` are mathematically special for the
> > > built-in types, but surely `x -= x` and `x ^= x` and `x /= x` are just as
> > > likely to indicate programmer error. I would be happy if Clang either
> > > took the philosophical stance "We will diagnose `x = x` but uniformly
> > > //never// `x op= x`," or else took the pragmatic stance "We will diagnose
> > > any `x op= x` or `x op x` that seems likely to be a programming bug."
> > > This "middle way" of warning only for `&=` and `|=` is bothersome to me.
> > I think "we want to diagnose anything that seems likely to be a programming
> > bug" is already our policy here. It's inevitable that we'll overlook
> > examples of that. I agree that we should apply this warning to at least
> > -=, ^=, and /=.
> Ok, will extend.
Would it make sense to also check `%=`?
It's a direct counterpart to `/=`, so i guess it would?
================
Comment at: lib/Sema/SemaExpr.cpp:12093
+ break;
+ }
+
----------------
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()`" ?
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
