[Sema] Add -Wno-self-assign-overloaded

[lebedev.ri]

Summary

It seems there isn't much enthusiasm for -wtest D45685.

This is more conservative version, which i had in the very first
revision of D44883, but that 'erroneously' got removed because of the review.

Based on some [irc] discussions, it must really be documented that
we want all the new diagnostics to have their own flags, to ease
rollouts, transitions, etc.

Please do note that i'm only adding -Wno-self-assign-overloaded,
but not -Wno-self-assign-field-overloaded, because i'm honestly
not aware of any false-positives from the -field variant,
but i can just as easily add it if wanted.
https://reviews.llvm.org/D44883#1068561

[lebedev.ri]

Ping. At least one of these needs to land.

[dblaikie]

lib/Sema/SemaExpr.cpp

11527–11528 ↗

(On Diff #142917)

Presumably this also changes how the warning is enabled? But that doesn't seem to be tested in this patch?

[lebedev.ri]

lib/Sema/SemaExpr.cpp

11527–11528 ↗

(On Diff #142917)

What testing do you have in mind?
The test/SemaCXX/warn-self-assign-overloaded.cpp change was supposed to show how it is enabled..

[dblaikie]

lib/Sema/SemaExpr.cpp

11527–11528 ↗

(On Diff #142917)

ah, misread those - figured they were testing the negative case (given the name of this patch) but I see they're testing the positive case.

Maybe testing the negative case would be useful too?

[lebedev.ri]

Add negative tests, too.

[lebedev.ri]

lib/Sema/SemaExpr.cpp

11527–11528 ↗

(On Diff #142917)

Added & committed.
Thank you for the review!

[dblaikie]

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

[rjmccall]

In D45766#1076090, @dblaikie wrote:

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

That's why I was suggesting putting it in the -w namespace. We really wouldn't expect or want users to ever use a *positive* version of this warning option — specifically asking for just the warnings that are known to be problematic for test code, across all warnings. It's just not really a warning group.

It could also be something like -fsuppress-problematic-test-warnings, of course, but I was basically thinking of -w as meaning -fsuppress-problematic-*-warnings.

[dblaikie]

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

I mean, I could imagine it might make more sense to default these warnings
off & users can turn them on for non-test code, potentially? So
"-Wnon-test" might make sense.

[rjmccall]

In D45766#1076176, @dblaikie wrote:

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

No. This would be novel.

I mean, I could imagine it might make more sense to default these warnings
off & users can turn them on for non-test code, potentially? So
"-Wnon-test" might make sense.

That's an interesting idea, but it's still not a warning group, because you shouldn't get the self-assign warnings unless -Wself-assign is enabled.

[ksu.shadura]

Hi,

we see the false-positive behavior of -Wno-self-assign-overloaded flag in case of subtraction assignment operator.
The minimal reproducer that we got: https://godbolt.org/g/8PQMpR

typedef int Int_t;
typedef double Double_t;
class TObject {};
template <class Element> class TMatrixTBase : public TObject {};
template <class Element> class TMatrixT : public TMatrixTBase<Element> {
public:
  enum EMatrixCreatorsOp1 {
    kZero,
    kUnit,
    kTransposed,
  };
  TMatrixT(){};
  TMatrixT(Int_t row_lwb, Int_t row_upb, Int_t col_lwb, Int_t col_upb){};
  TMatrixT(EMatrixCreatorsOp1 op, const TMatrixT &prototype){};
  TMatrixT<Element> &operator-=(const TMatrixT<Element> &source){return *this;};
};

typedef TMatrixT<Double_t> TMatrixD;
void stress_binary_ebe_op(Int_t rsize, Int_t csize) {
  TMatrixD m(2, rsize + 1, 0, csize - 1);
  TMatrixD m1(TMatrixD::kZero, m);
  m1 -= m1;
}

Thanks in advance!

[lebedev.ri]

In D45766#1097736, @ksu.shadura wrote:

Hi,

Hi.

we see the false-positive behavior of -Wno-self-assign-overloaded flag in case of subtraction assignment operator.
The minimal reproducer that we got: https://godbolt.org/g/8PQMpR
...

Could you please clarify, what is the false-positive?
The fact that it fires on overloaded -= is intended behavior, and is tested in tests, see cfe/trunk/test/SemaCXX/warn-self-assign-overloaded.cpp:

#ifndef DUMMY
  a *= a;
  a /= a; // expected-warning {{explicitly assigning}}
  a %= a; // expected-warning {{explicitly assigning}}
  a += a;
  a -= a; // expected-warning {{explicitly assigning}}
  a <<= a;
  a >>= a;
  a &= a; // expected-warning {{explicitly assigning}}
  a |= a; // expected-warning {{explicitly assigning}}
  a ^= a; // expected-warning {{explicitly assigning}}
#endif

Thanks in advance!

[ksu.shadura]

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

[lebedev.ri]

In D45766#1097797, @ksu.shadura wrote:

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?

In our case it is actually matrix, and subtraction of matrices is not commutative operation..

With that context, i do agree that it is a false-positive.
Please file a bug (do we already have a 7.0 metabug yet?).

I'm not too sure what is the best way to solve this.
The most obvious solution would be taking the operator status into account,
like it was considered during review, https://reviews.llvm.org/D44883#1048520,
https://godbolt.org/g/7K7Uwy - and if it is "non_trivial",
then issue it under -Wself-assign-overloaded-nontrivial ?

[• Quuxplusone]

In D45766#1097797, @ksu.shadura wrote:

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

Mathematical commutativity has nothing to do with this diagnostic. The diagnostic is complaining that m1 -= m1 produces a trivial effect (in this case m1.clear()) via a syntax that might indicate that the programmer made a typo (i.e. the compiler suspects that you meant m -= m1 or something).
From the function name stress_..., I infer that this is a unit-test. This warning is known to give "false positives" in unit-test code, where one often actually wants to achieve trivial effects through complicated code. The current recommendation there, AFAIK, is either to write something like

m1 -= static_cast<TMatrixD&>(m1);  // hide the self-subtraction from the compiler

or else to add -Wno-self-assign-overloaded to your compiler flags. (libc++'s test suite already passes -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-conversion -Wno-unused-local-typedef -Wno-#warnings, for example.)
(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

[ksu.shadura]

In D45766#1098823, @Quuxplusone wrote:

In D45766#1097797, @ksu.shadura wrote:

Thank you for the test example! I got your point, but I wanted to ask if it should be like this for commutative operations?
In our case it is actually matrix, and subtraction of matrices is not commutative operation..

Mathematical commutativity has nothing to do with this diagnostic. The diagnostic is complaining that m1 -= m1 produces a trivial effect (in this case m1.clear()) via a syntax that might indicate that the programmer made a typo (i.e. the compiler suspects that you meant m -= m1 or something).
From the function name stress_..., I infer that this is a unit-test. This warning is known to give "false positives" in unit-test code, where one often actually wants to achieve trivial effects through complicated code. The current recommendation there, AFAIK, is either to write something like

m1 -= static_cast<TMatrixD&>(m1);  // hide the self-subtraction from the compiler

or else to add -Wno-self-assign-overloaded to your compiler flags. (libc++'s test suite already passes -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-conversion -Wno-unused-local-typedef -Wno-#warnings, for example.)
(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

Thanks for your opinion, your guess was right! It is a unit-test, so seems we need to try to suppress warnings on our side.

[lebedev.ri]

In D45766#1101533, @ksu.shadura wrote:

Thanks for your opinion, your guess was right! It is a unit-test, so seems we need to try to suppress warnings on our side.

Ok, great that this time it got resolved this trivially!

In D45766#1098823, @Quuxplusone wrote:

(That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in -Wall...)

Too bad there is no way to track how many true-positives it produces.
Anecdotal evidence: it just prevented me from wasting time trying to understand what is going on during D46602 development, so sure, it can have false-positive noise, but..

cfe/trunk/docs/ReleaseNotes.rst

 - ``-Wextra-semi`` now also controls ``-Wc++98-compat-extra-semi``.
   Please do note that if you pass ``-Wno-c++98-compat-pedantic``, it implies
   ``-Wno-c++98-compat-extra-semi``, so if you want that diagnostic, you need
   to explicitly re-enable it (e.g. by appending ``-Wextra-semi``).
 
 - ``-Wself-assign`` and ``-Wself-assign-field`` were extended to diagnose
   self-assignment operations using overloaded operators (i.e. classes).
   If you are doing such an assignment intentionally, e.g. in a unit test for
-  a data structure, the warning can be suppressed by adding ``*&`` to the
-  right-hand side or casting it to the appropriate reference type.
+  a data structure, the first warning can be disabled by passing
+  ``-Wno-self-assign-overloaded``, also the warning can be suppressed by adding
+  ``*&`` to the right-hand side or casting it to the appropriate reference type.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Clang binary and libraries have been renamed from 7.0 to 7.
   For example, the ``clang`` binary will be called ``clang-7``
   instead of ``clang-7.0``.
 

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def RedundantMove : DiagGroup<"redundant-move">;
 def Register : DiagGroup<"register", [DeprecatedRegister]>;
 def ReturnStackAddress : DiagGroup<"return-stack-address">;
 def ReturnTypeCLinkage : DiagGroup<"return-type-c-linkage">;
 def ReturnType : DiagGroup<"return-type", [ReturnTypeCLinkage]>;
 def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy",
                                     [CXX98CompatBindToTemporaryCopy]>;
 def SelfAssignmentField : DiagGroup<"self-assign-field">;
-def SelfAssignment : DiagGroup<"self-assign", [SelfAssignmentField]>;
+def SelfAssignmentOverloaded : DiagGroup<"self-assign-overloaded">;
+def SelfAssignment : DiagGroup<"self-assign", [SelfAssignmentOverloaded, SelfAssignmentField]>;
 def SelfMove : DiagGroup<"self-move">;
 def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
 def Sentinel : DiagGroup<"sentinel">;
 def MissingMethodReturnType : DiagGroup<"missing-method-return-type">;
 
 def ShadowField : DiagGroup<"shadow-field">;
 def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">;
 def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor",

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_overloaded_shift_in_comparison :Warning<
   InGroup<OverloadedShiftOpParentheses>;
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>;
 
-def warn_self_assignment : Warning<
+def warn_self_assignment_builtin : Warning<
   "explicitly assigning value of variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
+def warn_self_assignment_overloaded : Warning<
+  "explicitly assigning value of variable of type %0 to itself">,
+  InGroup<SelfAssignmentOverloaded>, DefaultIgnore;
 def warn_self_move : Warning<
   "explicitly moving variable of type %0 to itself">,
   InGroup<SelfMove>, DefaultIgnore;
 
 def warn_redundant_move_on_return : Warning<
   "redundant move in return statement">,
   InGroup<RedundantMove>, DefaultIgnore;
 def warn_pessimizing_move_on_return : Warning<

cfe/trunk/lib/Sema/SemaExpr.cpp

static inline UnaryOperatorKind ConvertTokenKindToUnaryOpcode(
   case tok::kw___extension__: Opc = UO_Extension; break;
   }
   return Opc;
 }
 
 /// DiagnoseSelfAssignment - Emits a warning if a value is assigned to itself.
 /// This warning suppressed in the event of macro expansions.
 static void DiagnoseSelfAssignment(Sema &S, Expr *LHSExpr, Expr *RHSExpr,
-                                   SourceLocation OpLoc) {
+                                   SourceLocation OpLoc, bool IsBuiltin) {
   if (S.inTemplateInstantiation())
     return;
   if (S.isUnevaluatedContext())
     return;
   if (OpLoc.isInvalid() || OpLoc.isMacroID())
     return;
   LHSExpr = LHSExpr->IgnoreParenImpCasts();
   RHSExpr = RHSExpr->IgnoreParenImpCasts();
static void DiagnoseSelfAssignment(Sema &S, Expr *LHSExpr, Expr *RHSExpr,
   if (LHSDecl != RHSDecl)
     return;
   if (LHSDecl->getType().isVolatileQualified())
     return;
   if (const ReferenceType *RefTy = LHSDecl->getType()->getAs<ReferenceType>())
     if (RefTy->getPointeeType().isVolatileQualified())
       return;
 
-  S.Diag(OpLoc, diag::warn_self_assignment)
-      << LHSDeclRef->getType()
-      << LHSExpr->getSourceRange() << RHSExpr->getSourceRange();
+  S.Diag(OpLoc, IsBuiltin ? diag::warn_self_assignment_builtin
+                          : diag::warn_self_assignment_overloaded)
+      << LHSDeclRef->getType() << LHSExpr->getSourceRange()
+      << RHSExpr->getSourceRange();
 }
 
 /// Check if a bitwise-& is performed on an Objective-C pointer.  This
 /// is usually indicative of introspection within the Objective-C pointer.
 static void checkObjCPointerIntrospection(Sema &S, ExprResult &L, ExprResult &R,
                                           SourceLocation OpLoc) {
   if (!S.getLangOpts().ObjC1)
     return;
ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
   case BO_Assign:
     ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, QualType());
     if (getLangOpts().CPlusPlus &&
         LHS.get()->getObjectKind() != OK_ObjCProperty) {
       VK = LHS.get()->getValueKind();
       OK = LHS.get()->getObjectKind();
     }
     if (!ResultTy.isNull()) {
-      DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
+      DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc, true);
       DiagnoseSelfMove(LHS.get(), RHS.get(), OpLoc);
     }
     RecordModifiableNonNullParam(*this, LHS.get());
     break;
   case BO_PtrMemD:
   case BO_PtrMemI:
     ResultTy = CheckPointerToMemberOperands(LHS, RHS, VK, OpLoc,
                                             Opc == BO_PtrMemI);
ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
   case BO_ShrAssign:
     CompResultTy = CheckShiftOperands(LHS, RHS, OpLoc, Opc, true);
     CompLHSTy = CompResultTy;
     if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid())
       ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy);
     break;
   case BO_AndAssign:
   case BO_OrAssign: // fallthrough
-    DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
+    DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc, true);
     LLVM_FALLTHROUGH;
   case BO_XorAssign:
     CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
     CompLHSTy = CompResultTy;
     if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid())
       ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy);
     break;
   case BO_Comma:
static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
   switch (Opc) {
   case BO_Assign:
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-    DiagnoseSelfAssignment(S, LHS, RHS, OpLoc);
+    DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
     CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
     break;
   default:
     break;
   }
 
   // Find all of the overloaded operators visible from this
   // point. We perform both an operator-name lookup from the local

cfe/trunk/test/SemaCXX/warn-self-assign-overloaded-disable.cpp

+// RUN: %clang_cc1 -fsyntax-only -Wall -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wno-self-assign-overloaded -DSILENCE -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-overloaded -DSILENCE -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -Wno-self-assign-overloaded -DSILENCE -verify %s
+
+struct S {};
+
+void f() {
+  S a;
+#ifndef SILENCE
+  a = a; // expected-warning{{explicitly assigning}}
+#else
+  // expected-no-diagnostics
+  a = a;
+#endif
+}

cfe/trunk/test/SemaCXX/warn-self-assign-overloaded.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DDUMMY -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DV0 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DV1 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DV2 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DV3 -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wself-assign -DV4 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DDUMMY -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DV0 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DV1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DV2 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DV3 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-self-assign -Wself-assign-overloaded -DV4 -verify %s
 
 #ifdef DUMMY
 struct S {};
 #else
 struct S {
 #if defined(V0)
   S() = default;
 #elif defined(V1)