Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -59,6 +59,12 @@ ``-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. + Non-comprehensive list of changes in this release ------------------------------------------------- Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -10701,12 +10701,34 @@ static void CheckIdentityFieldAssignment(Expr *LHSExpr, Expr *RHSExpr, SourceLocation Loc, Sema &Sema) { + if (Sema.inTemplateInstantiation()) + return; + if (Sema.isUnevaluatedContext()) + return; + if (Loc.isInvalid() || Loc.isMacroID()) + return; + if (LHSExpr->getExprLoc().isMacroID() || RHSExpr->getExprLoc().isMacroID()) + return; + // C / C++ fields MemberExpr *ML = dyn_cast(LHSExpr); MemberExpr *MR = dyn_cast(RHSExpr); - if (ML && MR && ML->getMemberDecl() == MR->getMemberDecl()) { - if (isa(ML->getBase()) && isa(MR->getBase())) - Sema.Diag(Loc, diag::warn_identity_field_assign) << 0; + if (ML && MR) { + if (!(isa(ML->getBase()) && isa(MR->getBase()))) + return; + const ValueDecl *LHSDecl = + cast(ML->getMemberDecl()->getCanonicalDecl()); + const ValueDecl *RHSDecl = + cast(MR->getMemberDecl()->getCanonicalDecl()); + if (LHSDecl != RHSDecl) + return; + if (LHSDecl->getType().isVolatileQualified()) + return; + if (const ReferenceType *RefTy = LHSDecl->getType()->getAs()) + if (RefTy->getPointeeType().isVolatileQualified()) + return; + + Sema.Diag(Loc, diag::warn_identity_field_assign) << 0; } // Objective-C instance variables @@ -11460,12 +11482,13 @@ } /// 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) { if (S.inTemplateInstantiation()) return; + if (S.isUnevaluatedContext()) + return; if (OpLoc.isInvalid() || OpLoc.isMacroID()) return; LHSExpr = LHSExpr->IgnoreParenImpCasts(); @@ -12080,6 +12103,21 @@ static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc, BinaryOperatorKind Opc, Expr *LHS, Expr *RHS) { + 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); + 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 // scope and an argument-dependent lookup based on the types of Index: test/SemaCXX/member-init.cpp =================================================================== --- test/SemaCXX/member-init.cpp +++ test/SemaCXX/member-init.cpp @@ -101,7 +101,7 @@ struct Sprite { Point location = Point(0,0); // expected-error {{no matching constructor for initialization of 'rdar14084171::Point'}} }; - void f(Sprite& x) { x = x; } + void f(Sprite& x) { x = x; } // expected-warning {{explicitly assigning value of variable}} } namespace PR18560 { Index: test/SemaCXX/warn-self-assign-builtin.cpp =================================================================== --- test/SemaCXX/warn-self-assign-builtin.cpp +++ test/SemaCXX/warn-self-assign-builtin.cpp @@ -0,0 +1,67 @@ +// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s + +void f() { + int a = 42, b = 42; + 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; + + a *= a; + a /= a; + a %= a; + a += a; + a -= a; + a <<= a; + a >>= a; + a &= a; // expected-warning {{explicitly assigning}} + a |= a; // expected-warning {{explicitly assigning}} + a ^= a; +} + +// Dummy type. +struct S {}; + +void false_positives() { +#define OP = +#define LHS a +#define RHS a + int a = 42; + // 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 + + // A way to silence the warning. + a = (int &)a; + + // Volatile stores aren't side-effect free. + volatile int vol_a; + vol_a = vol_a; + volatile int &vol_a_ref = vol_a; + vol_a_ref = vol_a_ref; +} + +// Do not diagnose self-assigment in an unevaluated context +void false_positives_unevaluated_ctx(int a) noexcept(noexcept(a = a)) // expected-warning {{expression with side effects has no effect in an unevaluated context}} +{ + decltype(a = a) b = a; // expected-warning {{expression with side effects has no effect in an unevaluated context}} + static_assert(noexcept(a = a), ""); // expected-warning {{expression with side effects has no effect in an unevaluated context}} + static_assert(sizeof(a = a), ""); // expected-warning {{expression with side effects has no effect in an unevaluated context}} +} + +template +void g() { + T a; + a = a; // expected-warning{{explicitly assigning}} +} +void instantiate() { + g(); + g(); +} Index: test/SemaCXX/warn-self-assign-field-builtin.cpp =================================================================== --- test/SemaCXX/warn-self-assign-field-builtin.cpp +++ test/SemaCXX/warn-self-assign-field-builtin.cpp @@ -0,0 +1,117 @@ +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s + +struct C { + int a; + int b; + + void f() { + a = a; // expected-warning {{assigning field to itself}} + b = b; // expected-warning {{assigning field to itself}} + a = b; + + this->a = a; // expected-warning {{assigning field to itself}} + this->b = b; // expected-warning {{assigning field to itself}} + a = this->a; // expected-warning {{assigning field to itself}} + b = this->b; // expected-warning {{assigning field to itself}} + this->a = this->a; // expected-warning {{assigning field to itself}} + this->b = this->b; // expected-warning {{assigning field to itself}} + + a = b; + a = this->b; + this->a = b; + this->a = this->b; + + a *= a; + a /= a; + a %= a; + a += a; + a -= a; + a <<= a; + a >>= a; + a &= a; + a |= a; + a ^= a; + } + + void false_positives() { +#define OP = +#define LHS a +#define RHS 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 + + // A way to silence the warning. + a = (int &)a; + } + + // Do not diagnose self-assigment in an unevaluated context + void false_positives_unevaluated_ctx() noexcept(noexcept(a = a)) // expected-warning {{expression with side effects has no effect in an unevaluated context}} + { + decltype(a = a) b = a; // expected-warning {{expression with side effects has no effect in an unevaluated context}} + static_assert(noexcept(a = a), ""); // expected-warning {{expression with side effects has no effect in an unevaluated context}} + static_assert(sizeof(a = a), ""); // expected-warning {{expression with side effects has no effect in an unevaluated context}} + } + + volatile int vol_a; + void vol_test() { + // Volatile stores aren't side-effect free. + vol_a = vol_a; + volatile int &vol_a_ref = vol_a; + vol_a_ref = vol_a_ref; + } +}; + +// Dummy type. +struct Dummy {}; + +template +struct TemplateClass { + T var; + void f() { + var = var; // expected-warning {{assigning field to itself}} + } +}; +void instantiate() { + { + TemplateClass c; + c.f(); + } + { + TemplateClass c; + c.f(); + } +} + +// It may make sense not to warn on the rest of the tests. +// It may be a valid use-case to self-assign to tell the compiler that +// it is ok to vectorize the store. + +void f0(C *s, C *t) { + s->a = s->a; + t->a = s->a; +} + +void f1(C &s, C &t) { + s.a = s.a; + t.a = s.a; +} + +struct T { + C *s; +}; + +void f2(T *t, T *t2) { + t->s->a = t->s->a; + t2->s->a = t->s->a; +} + +void f3(T &t, T &t2) { + t.s->a = t.s->a; + t2.s->a = t.s->a; +} Index: test/SemaCXX/warn-self-assign-field-overloaded.cpp =================================================================== --- test/SemaCXX/warn-self-assign-field-overloaded.cpp +++ test/SemaCXX/warn-self-assign-field-overloaded.cpp @@ -0,0 +1,162 @@ +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DDUMMY -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DV0 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DV1 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DV2 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DV3 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -DV4 -verify %s + +#ifdef DUMMY +struct S {}; +#else +struct S { +#if defined(V0) + S() = default; +#elif defined(V1) + S &operator=(const S &) = default; +#elif defined(V2) + S &operator=(S &) = default; +#elif defined(V3) + S &operator=(const S &); +#elif defined(V4) + S &operator=(S &); +#else +#error Define something! +#endif + S &operator*=(const S &); + S &operator/=(const S &); + S &operator%=(const S &); + S &operator+=(const S &); + S &operator-=(const S &); + S &operator<<=(const S &); + S &operator>>=(const S &); + S &operator&=(const S &); + S &operator|=(const S &); + S &operator^=(const S &); + S &operator=(const volatile S &) volatile; +}; +#endif +struct C { + S a; + S b; + + void f() { + a = a; // expected-warning {{assigning field to itself}} + b = b; // expected-warning {{assigning field to itself}} + a = b; + + this->a = a; // expected-warning {{assigning field to itself}} + this->b = b; // expected-warning {{assigning field to itself}} + a = this->a; // expected-warning {{assigning field to itself}} + b = this->b; // expected-warning {{assigning field to itself}} + this->a = this->a; // expected-warning {{assigning field to itself}} + this->b = this->b; // expected-warning {{assigning field to itself}} + + a = b; + a = this->b; + this->a = b; + this->a = this->b; + +#ifndef DUMMY + a *= a; + a /= a; // expected-warning {{assigning field to itself}} + a %= a; // expected-warning {{assigning field to itself}} + a += a; + a -= a; // expected-warning {{assigning field to itself}} + 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}} +#endif + } + + void false_positives() { +#define OP = +#define LHS a +#define RHS 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 + + // Ways to silence the warning. + a = *&a; + a = (S &)a; + a = static_cast(a); + } + +#ifndef DUMMY + volatile S vol_a; + void vol_test() { + // Volatile stores aren't side-effect free. + vol_a = vol_a; + volatile S &vol_a_ref = vol_a; + vol_a_ref = vol_a_ref; + } +#endif +}; + +// Do not diagnose self-assigment in an unevaluated context +struct SNoExcept { + SNoExcept() = default; + SNoExcept &operator=(const SNoExcept &) noexcept; +}; +struct false_positives_unevaluated_ctx_class { + SNoExcept a; + + void false_positives_unevaluated_ctx(SNoExcept a) noexcept(noexcept(a = a)) { + decltype(a = a) b = a; + static_assert(noexcept(a = a), ""); + static_assert(sizeof(a = a), ""); + } +}; + +template +struct TemplateClass { + T var; + void f() { + var = var; // expected-warning {{assigning field to itself}} + } +}; +void instantiate() { + { + TemplateClass c; + c.f(); + } + { + TemplateClass c; + c.f(); + } +} + +// It may make sense not to warn on the rest of the tests. +// It may be a valid use-case to self-assign to tell the compiler that +// it is ok to vectorize the store. + +void f0(C *s, C *t) { + s->a = s->a; + t->a = s->a; +} + +void f1(C &s, C &t) { + s.a = s.a; + t.a = s.a; +} + +struct T { + C *s; +}; + +void f2(T *t, T *t2) { + t->s->a = t->s->a; + t2->s->a = t->s->a; +} + +void f3(T &t, T &t2) { + t.s->a = t.s->a; + t2.s->a = t.s->a; +} Index: test/SemaCXX/warn-self-assign-overloaded.cpp =================================================================== --- test/SemaCXX/warn-self-assign-overloaded.cpp +++ test/SemaCXX/warn-self-assign-overloaded.cpp @@ -0,0 +1,109 @@ +// 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 + +#ifdef DUMMY +struct S {}; +#else +struct S { +#if defined(V0) + S() = default; +#elif defined(V1) + S &operator=(const S &) = default; +#elif defined(V2) + S &operator=(S &) = default; +#elif defined(V3) + S &operator=(const S &); +#elif defined(V4) + S &operator=(S &); +#else +#error Define something! +#endif + S &operator*=(const S &); + S &operator/=(const S &); + S &operator%=(const S &); + S &operator+=(const S &); + S &operator-=(const S &); + S &operator<<=(const S &); + S &operator>>=(const S &); + 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; + 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 +} + +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 + + // Ways to silence the warning. + a = *&a; + a = (S &)a; + a = static_cast(a); + +#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 +} + +// Do not diagnose self-assigment in an unevaluated context +struct SNoExcept { + SNoExcept() = default; + SNoExcept &operator=(const SNoExcept &) noexcept; +}; +void false_positives_unevaluated_ctx(SNoExcept a) noexcept(noexcept(a = a)) { + decltype(a = a) b = a; + static_assert(noexcept(a = a), ""); + static_assert(sizeof(a = a), ""); +} + +template +void g() { + T a; + a = a; // expected-warning{{explicitly assigning}} +} +void instantiate() { + g(); + g(); +} Index: test/SemaCXX/warn-self-assign.cpp =================================================================== --- test/SemaCXX/warn-self-assign.cpp +++ test/SemaCXX/warn-self-assign.cpp @@ -1,50 +0,0 @@ -// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s - -void f() { - int a = 42, b = 42; - 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; - a &= a; // expected-warning{{explicitly assigning}} - a |= a; // expected-warning{{explicitly assigning}} - a ^= a; -} - -// Dummy type. -struct S {}; - -void false_positives() { -#define OP = -#define LHS a -#define RHS a - int a = 42; - // 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 - - S s; - s = s; // Not a builtin assignment operator, no warning. - - // Volatile stores aren't side-effect free. - volatile int vol_a; - vol_a = vol_a; - volatile int &vol_a_ref = vol_a; - vol_a_ref = vol_a_ref; -} - -template void g() { - T a; - a = a; // May or may not be a builtin assignment operator, no warning. -} -void instantiate() { - g(); - g(); -}