lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, rsmith, rtrieu, nikola, chandlerc,
rjmccall.
This has just bit me, so i though it would be nice to avoid that next time :)
Motivational case:
https://godbolt.org/g/cq9UNk
Basically, it's likely to happen if you don't like shadowing issues,
and use `-Wshadow` and friends. And it won't be diagnosed by clang.
The reason is, these self-assign diagnostics only work for builtin assignment
operators. Which makes sense, one could have a very special operator=,
that does something unusual in case of self-assignment,
so it may make sense to not warn on that.
But while it may be intentional in some cases, it may be a bug in other cases,
so it would be really great to have some [opt-in] diagnostic about it...
For now, this diff restructures `SelfAssignment` diag group, splits it into
`SelfAssignmentBuiltin` (as what is already in trunk),
and the new `SelfAssignmentOverloaded`, which is not enabled by default/`-Wall`.
For now i have put it into `-Wextra`, but i don't know if it should be there.
Additionally, a question: what should be done for trivial assignment operators?
Perhaps this should be split even more, and if the operator is trivial implicit
(or even trivial user-defined =default ?), it should even warn in `-Wall`?
I'm guessing the self-move, and self-field-assign warnings should also be
adjusted
to work for overloaded operators, but i did not want to bloat this diff.
Repository:
rC Clang
https://reviews.llvm.org/D44883
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/SemaCXX/warn-self-assign-builtin.cpp
test/SemaCXX/warn-self-assign-overloaded.cpp
test/SemaCXX/warn-self-assign.cpp
Index: test/SemaCXX/warn-self-assign-overloaded.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/warn-self-assign-overloaded.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-builtin -DDUMMY -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -DDUMMY -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-builtin -DV0 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -DV0 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-builtin -DV1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -DV1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-builtin -DV2 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -DV2 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-builtin -DV3 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-overloaded -DV3 -verify %s
+
+#ifdef DUMMY
+struct S {};
+#else
+struct S {
+#if defined(V0)
+ S &operator=(const S &) = default;
+#elif defined(V1)
+ S &operator=(S &) = default;
+#elif defined(V2)
+ S &operator=(const S &);
+#elif defined(V3)
+ S &operator=(S &);
+#else
+#error Define something!
+#endif
+ S &operator&=(const S &);
+ S &operator|=(const S &);
+ S &operator^=(const S &);
+ S &operator=(const volatile S &) volatile;
+};
+#endif
+
+void f() {
+ S a, b;
+ a = a; // expected-warning{{explicitly assigning}}
+ b = b; // expected-warning{{explicitly assigning}}
+ a = b;
+ b = a = b;
+ a = a = a; // expected-warning{{explicitly assigning}}
+ a = b = b = a;
+#ifndef DUMMY
+ a &= a; // expected-warning{{explicitly assigning}}
+ a |= a; // expected-warning{{explicitly assigning}}
+ a ^= a;
+#endif
+}
+
+void false_positives() {
+#define OP =
+#define LHS a
+#define RHS a
+ S a;
+ // These shouldn't warn due to the use of the preprocessor.
+ a OP a;
+ LHS = a;
+ a = RHS;
+ LHS OP RHS;
+#undef OP
+#undef LHS
+#undef RHS
+
+#ifndef DUMMY
+ // Volatile stores aren't side-effect free.
+ volatile S vol_a;
+ vol_a = vol_a;
+ volatile S &vol_a_ref = vol_a;
+ vol_a_ref = vol_a_ref;
+#endif
+}
+
+template <typename T>
+void g() {
+ T a;
+ a = a; // expected-warning{{explicitly assigning}}
+}
+void instantiate() {
+ g<int>();
+ g<S>();
+}
Index: test/SemaCXX/warn-self-assign-builtin.cpp
===================================================================
--- test/SemaCXX/warn-self-assign-builtin.cpp
+++ test/SemaCXX/warn-self-assign-builtin.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-self-assign-overloaded -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-builtin -verify %s
void f() {
int a = 42, b = 42;
@@ -40,7 +41,8 @@
vol_a_ref = vol_a_ref;
}
-template <typename T> void g() {
+template <typename T>
+void g() {
T a;
a = a; // May or may not be a builtin assignment operator, no warning.
}
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10698,14 +10698,13 @@
}
static void CheckIdentityFieldAssignment(Expr *LHSExpr, Expr *RHSExpr,
- SourceLocation Loc,
- Sema &Sema) {
+ SourceLocation Loc, Sema &Sema) {
// C / C++ fields
MemberExpr *ML = dyn_cast<MemberExpr>(LHSExpr);
MemberExpr *MR = dyn_cast<MemberExpr>(RHSExpr);
if (ML && MR && ML->getMemberDecl() == MR->getMemberDecl()) {
if (isa<CXXThisExpr>(ML->getBase()) && isa<CXXThisExpr>(MR->getBase()))
- Sema.Diag(Loc, diag::warn_identity_field_assign) << 0;
+ Sema.Diag(Loc, diag::warn_identity_field_assign_builtin) << 0;
}
// Objective-C instance variables
@@ -10715,7 +10714,7 @@
DeclRefExpr *RL = dyn_cast<DeclRefExpr>(OL->getBase()->IgnoreImpCasts());
DeclRefExpr *RR = dyn_cast<DeclRefExpr>(OR->getBase()->IgnoreImpCasts());
if (RL && RR && RL->getDecl() == RR->getDecl())
- Sema.Diag(Loc, diag::warn_identity_field_assign) << 1;
+ Sema.Diag(Loc, diag::warn_identity_field_assign_builtin) << 1;
}
}
@@ -11459,10 +11458,9 @@
}
/// DiagnoseSelfAssignment - Emits a warning if a value is assigned to itself.
-/// This warning is only emitted for builtin assignment operations. It is also
-/// suppressed in the event of macro expansions.
+/// 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 (OpLoc.isInvalid() || OpLoc.isMacroID())
@@ -11487,9 +11485,11 @@
if (RefTy->getPointeeType().isVolatileQualified())
return;
- S.Diag(OpLoc, diag::warn_self_assignment)
- << LHSDeclRef->getType()
- << LHSExpr->getSourceRange() << RHSExpr->getSourceRange();
+ // FIXME: what do we want to do with trivial operator=()? treat it as builtin?
+ 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
@@ -11682,7 +11682,8 @@
OK = LHS.get()->getObjectKind();
}
if (!ResultTy.isNull()) {
- DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
+ DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc,
+ /*IsBuiltin=*/true);
DiagnoseSelfMove(LHS.get(), RHS.get(), OpLoc);
}
RecordModifiableNonNullParam(*this, LHS.get());
@@ -11780,7 +11781,8 @@
break;
case BO_AndAssign:
case BO_OrAssign: // fallthrough
- DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
+ DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc,
+ /*IsBuiltin=*/true);
LLVM_FALLTHROUGH;
case BO_XorAssign:
CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
@@ -12079,6 +12081,16 @@
static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
BinaryOperatorKind Opc,
Expr *LHS, Expr *RHS) {
+ switch (Opc) {
+ case BO_Assign:
+ case BO_AndAssign:
+ case BO_OrAssign:
+ DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, /*IsBuiltin=*/false);
+ break;
+ default:
+ break;
+ }
+
// Find all of the overloaded operators visible from this
// point. We perform both an operator-name lookup from the local
// scope and an argument-dependent lookup based on the types of
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5601,9 +5601,12 @@
"operator '%0' has lower precedence than '%1'; "
"'%1' will be evaluated first">, InGroup<ShiftOpParentheses>;
-def warn_self_assignment : Warning<
+def warn_self_assignment_overloaded : Warning<
+ "explicitly assigning value of variable of class type %0 to itself">,
+ InGroup<SelfAssignmentOverloaded>, DefaultIgnore;
+def warn_self_assignment_builtin : Warning<
"explicitly assigning value of variable of type %0 to itself">,
- InGroup<SelfAssignment>, DefaultIgnore;
+ InGroup<SelfAssignmentBuiltin>, DefaultIgnore;
def warn_self_move : Warning<
"explicitly moving variable of type %0 to itself">,
InGroup<SelfMove>, DefaultIgnore;
@@ -7925,9 +7928,9 @@
"unspecified (use strncmp instead)">,
InGroup<StringCompare>;
-def warn_identity_field_assign : Warning<
+def warn_identity_field_assign_builtin : Warning<
"assigning %select{field|instance variable}0 to itself">,
- InGroup<SelfAssignmentField>;
+ InGroup<SelfAssignmentBuiltinField>;
// Type safety attributes
def err_type_tag_for_datatype_not_ice : Error<
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -399,8 +399,11 @@
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 SelfAssignmentBuiltinField : DiagGroup<"self-assign-field">;
+def SelfAssignmentBuiltin : DiagGroup<"self-assign-builtin", [SelfAssignmentBuiltinField]>;
+def SelfAssignment : DiagGroup<"self-assign", [SelfAssignmentBuiltin,
+ SelfAssignmentOverloaded]>;
def SelfMove : DiagGroup<"self-move">;
def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
def Sentinel : DiagGroup<"sentinel">;
@@ -729,6 +732,7 @@
MissingFieldInitializers,
IgnoredQualifiers,
InitializerOverrides,
+ SelfAssignmentOverloaded,
SemiBeforeMethodBody,
MissingMethodReturnType,
SignCompare,
@@ -750,7 +754,7 @@
MultiChar,
Reorder,
ReturnType,
- SelfAssignment,
+ SelfAssignmentBuiltin,
SelfMove,
SizeofArrayArgument,
SizeofArrayDecay,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
