aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, dblaikie.
aaron.ballman added a subscriber: cfe-commits.
All copy operations were created equal (as far as the core language is
concerned), but some copy operations are more equal than others (as far as the
library is concerned). The same is true of move operations. This patch
diagnoses copy and move operations (constructors and assignment operators) that
are non-idiomatic so that users are not surprised when their copyable type
cannot be placed in a standard library container or used with a standard
library algorithm.
Idiomatic copy and move operations are (where T is the class type for the
object):
Copy Constructor: takes a const T&
Move constructor: takes a T&&
Copy Assignment: takes a const T&, returns a T&, and has no ref-qualifiers
Move Assignment: takes a T&&, returns a T&, and has no ref-qualifiers
One thing this patch does not have, but I would like to add, are fix-it hints
to correct the types involved. However, I cannot find a way to get the full
source range for the type, including qualifiers. For a parameter of type 'const
volatile T&', the TypeLoc appears to only track the location of T&, but not the
qualifiers. Assistance in understanding how to do this would be appreciated.
http://reviews.llvm.org/D15228
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/warn-non-idiomatic-copy-move.cpp
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
+};
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<CXXRecordDecl>(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<CXXRecordDecl>(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: 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: 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<NonIdiomaticCopyMove>;
+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<NonIdiomaticCopyMove>;
+
// These should be errors.
def warn_undefined_internal : Warning<
"%select{function|variable}0 %q1 has internal linkage but is not defined">,
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).
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
