Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -495,6 +495,7 @@ def GNUZeroLineDirective : DiagGroup<"gnu-zero-line-directive">; def GNUZeroVariadicMacroArguments : DiagGroup<"gnu-zero-variadic-macro-arguments">; def Fallback : DiagGroup<"fallback">; +def NonIdiomaticCopyMove : DiagGroup<"non-idiomatic-copy-move">; // This covers both the deprecated case (in C++98) // and the extension case (in C++11 onwards). Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -4162,6 +4162,15 @@ "%select{copy|move}0 assignment operator of %1 is implicitly deleted " "because field %2 is of %select{reference|const-qualified}4 type %3">; +def warn_non_idiomatic_copy_move_ctor : Warning< + "non-idiomatic %select{copy|move}0 constructor declaration; consider " + "%select{'const T&'|'T&&'}0 instead">, + InGroup; +def warn_non_idiomatic_copy_move_assign : Warning< + "non-idiomatic %select{copy|move}0 assignment operator declaration; consider " + "%select{returning 'T&'|'%select{const T&|T&&}0'|removing ref-qualifiers}1 " + "instead">, InGroup; + // These should be errors. def warn_undefined_internal : Warning< "%select{function|variable}0 %q1 has internal linkage but is not defined">, Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -5331,6 +5331,7 @@ QualType CheckConstructorDeclarator(Declarator &D, QualType R, StorageClass& SC); void CheckConstructor(CXXConstructorDecl *Constructor); + void CheckAssignmentOperatorOverload(CXXMethodDecl *Assignment); QualType CheckDestructorDeclarator(Declarator &D, QualType R, StorageClass& SC); bool CheckDestructor(CXXDestructorDecl *Destructor); Index: lib/Sema/SemaDeclCXX.cpp =================================================================== --- lib/Sema/SemaDeclCXX.cpp +++ lib/Sema/SemaDeclCXX.cpp @@ -6727,8 +6727,7 @@ } /// CheckConstructor - Checks a fully-formed constructor for -/// well-formedness, issuing any diagnostics required. Returns true if -/// the constructor declarator is invalid. +/// well-formedness, issuing any diagnostics required. void Sema::CheckConstructor(CXXConstructorDecl *Constructor) { CXXRecordDecl *ClassDecl = dyn_cast(Constructor->getDeclContext()); @@ -6746,9 +6745,10 @@ Constructor->getParamDecl(1)->hasDefaultArg())) && Constructor->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) { - QualType ParamType = Constructor->getParamDecl(0)->getType(); QualType ClassTy = Context.getTagDeclType(ClassDecl); - if (Context.getCanonicalType(ParamType).getUnqualifiedType() == ClassTy) { + unsigned Quals; + if (Context.getCanonicalType(Constructor->getParamDecl(0)->getType()) + .getUnqualifiedType() == ClassTy) { SourceLocation ParamLoc = Constructor->getParamDecl(0)->getLocation(); const char *ConstRef = Constructor->getParamDecl(0)->getIdentifier() ? "const &" @@ -6759,10 +6759,105 @@ // FIXME: Rather that making the constructor invalid, we should endeavor // to fix the type. Constructor->setInvalidDecl(); + } else if (!Constructor->isDeletedAsWritten() && + Constructor->isCopyConstructor(Quals) && + ((Quals & Qualifiers::Volatile) || + !(Quals & Qualifiers::Const))) { + // Warn about copy constructors of the form T&, volatile T&, or + // const volatile T&, because they are not idiomatic copy constructors. + // Do not warn about copy constructors that are deleted; presumably, the + // user knows what they are doing with those. + Diag(Constructor->getParamDecl(0)->getLocation(), + diag::warn_non_idiomatic_copy_move_ctor) + << /*copy*/ 0; + + // TODO: FixIt hint to correct the type + } else if (!Constructor->isDeletedAsWritten() && + Constructor->isMoveConstructor(Quals) && Quals) { + // Warn about move constructors of the form const T&&, volatile T&&, or + // const volatile T&&, because they are not idiomatic move constructors. + // Do not warn about move constructors that are deleted; presumably, the + // user knows what they are doing with those. + Diag(Constructor->getParamDecl(0)->getLocation(), + diag::warn_non_idiomatic_copy_move_ctor) << /*move*/1; + + // TODO: FixIt hint to correct the type } } } +/// \brief Checks a fully-formed assignment operator definition for +/// well-formedness, including any diagnostics required. +void Sema::CheckAssignmentOperatorOverload(CXXMethodDecl *Assignment) { + // Diagnose any non-idiomatic copy assignment operators. We determine that it + // is meant to be a copy assignment operator by looking at the parameter. + // Because we already know this is an assignment operator, we can look at the + // only parameter; if it is an rvalue reference, it is a move assignment. + // Anything else will be considered an attempt at a copy assignment operator, + // since those are more commonly used. + if (Assignment->isInvalidDecl()) + return; + + assert(Assignment->getNumParams() == 1 && "Invalid assignment operator"); + + CXXRecordDecl *ClassDecl = + dyn_cast(Assignment->getDeclContext()); + if (!ClassDecl) + return Assignment->setInvalidDecl(); + + if (!Assignment->isDeletedAsWritten()) { + QualType ParamType = Assignment->getParamDecl(0)->getType(); + QualType ReturnType = Assignment->getReturnType(); + QualType ClassTy = Context.getTagDeclType(ClassDecl); + QualType ParamNonRefTy = ParamType.getNonReferenceType(); + + if (Assignment->getRefQualifier() != RQ_None) { + // Diagnose a copy or move constructor if it has any ref qualifiers. + + // FIXME: this location isn't ideal, but the ref qualifier location + // information is not available. + Diag(Assignment->getLocation(), diag::warn_non_idiomatic_copy_move_assign) + << ParamType->isRValueReferenceType() << /*ref-qualifier*/ 2; + + // FIXME: FixIt hint to remove the ref qualifier. This requires knowing + // where the ref qualifier is written, however. + } else if (!ReturnType->isLValueReferenceType() || + ReturnType.getNonReferenceType().hasQualifiers() || + Context.getCanonicalType(ReturnType.getNonReferenceType()) + .getUnqualifiedType() != ClassTy) { + // Diagnose a copy or move constructor if its return type is anything but + // T&. + Diag(Assignment->getReturnTypeSourceRange().getBegin(), + diag::warn_non_idiomatic_copy_move_assign) + << ParamType->isRValueReferenceType() << /*return*/ 0; + + // TODO: FixIt hint to correct the return type. + } else if (Assignment->isMoveAssignmentOperator()) { + // Diagnose a move assignment operator that has qualifiers on its + // parameter. An rvalue reference type without qualifiers is idiomatic. + if (ParamNonRefTy.hasQualifiers()) + Diag(Assignment->getParamDecl(0)->getLocation(), + diag::warn_non_idiomatic_copy_move_assign) + << /*move*/ 1 << /*param*/ 1; + + // TODO: FixIt hint to correct the parameter type to T&&. + } else if (Assignment->isCopyAssignmentOperator()) { + // Diagnose a copy assignment operator that has a parameter type that is + // not an lvalue reference to an only-const object. + if (!ParamType->isLValueReferenceType() || + (ParamType->isLValueReferenceType() && + (ParamNonRefTy.isVolatileQualified() || + !ParamNonRefTy.isConstQualified()))) { + Diag(Assignment->getParamDecl(0)->getLocation(), + diag::warn_non_idiomatic_copy_move_assign) + << /*copy*/ 0 << /*param*/ 1; + + // TODO: FixIt hint to correct the parameter type to const T&. + } + } + } +} + /// CheckDestructor - Checks a fully-formed destructor definition for /// well-formedness, issuing any diagnostics required. Returns true /// on error. @@ -11640,6 +11735,9 @@ if (MethodDecl->isStatic()) return Diag(FnDecl->getLocation(), diag::err_operator_overload_static) << FnDecl->getDeclName(); + + if (Op == OO_Equal) + CheckAssignmentOperatorOverload(MethodDecl); } else { bool ClassOrEnumParam = false; for (auto Param : FnDecl->params()) { Index: test/SemaCXX/warn-non-idiomatic-copy-move.cpp =================================================================== --- test/SemaCXX/warn-non-idiomatic-copy-move.cpp +++ test/SemaCXX/warn-non-idiomatic-copy-move.cpp @@ -0,0 +1,86 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s + +struct A { + // Copy constructor + A(A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}} + A(volatile A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}} + A(const volatile A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}} + A(const A&); // ok + + // Move constructor + A(const A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}} + A(volatile A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}} + A(const volatile A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}} + A(A&&); // ok + + // Copy assignment + A& operator=(A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} + A& operator=(volatile A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} + A& operator=(const volatile A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} + A& operator=(A); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} + A& operator=(const A&); // ok + + // Move assignment + A& operator=(const A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}} + A& operator=(volatile A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}} + A& operator=(const volatile A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}} + A& operator=(A&&); // ok +}; + +struct B { + B&& operator=(const B&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}} + B&& operator=(B&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}} +}; + +struct C { + C operator=(const C&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}} + C operator=(C&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}} +}; + +struct D { + C& operator=(const D&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}} + C& operator=(D&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}} +}; + +struct E { + E& operator=(const E&) &; // expected-warning {{non-idiomatic copy assignment operator declaration; consider removing ref-qualifiers instead}} + E& operator=(E&&) &; // expected-warning {{non-idiomatic move assignment operator declaration; consider removing ref-qualifiers instead}} +}; + +struct F { + F(F&) = delete; // okay + F& operator=(F&) = default; // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} + F& operator=(const F&&) = delete; // okay +}; + +struct G { + G(G&) = default; // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}} + G(const G&, int = 0); // okay, but should be different diagnostic + +private: + G& operator=(G&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}} +}; + +struct H { +private: + H(H&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}} + H(const H&); // ok + H(const H&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}} + + void operator=(const H&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}} + void operator=(H&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}} + void operator=(H&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}} +}; + +struct I { + I(int); // ok + I(int&); // ok + I(int&&); // ok + I(H&); // ok + + I& operator=(int); // ok + I& operator=(int&); // ok + I& operator=(int&&); // ok + I& operator=(H&); // ok + I& operator=(H&&); // ok +};