diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp index b22fa94663d58..6a6e620a4387b 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp @@ -19,17 +19,86 @@ AST_MATCHER(FieldDecl, isMemberOfLambda) { return Node.getParent()->isLambda(); } +struct MemberFunctionInfo { + bool Declared{}; + bool Deleted{}; +}; + +struct MemberFunctionPairInfo { + MemberFunctionInfo Copy{}; + MemberFunctionInfo Move{}; +}; + +MemberFunctionPairInfo getConstructorsInfo(CXXRecordDecl const &Node) { + MemberFunctionPairInfo Constructors{}; + + for (CXXConstructorDecl const *Ctor : Node.ctors()) { + if (Ctor->isCopyConstructor()) { + Constructors.Copy.Declared = true; + if (Ctor->isDeleted()) + Constructors.Copy.Deleted = true; + } + if (Ctor->isMoveConstructor()) { + Constructors.Move.Declared = true; + if (Ctor->isDeleted()) + Constructors.Move.Deleted = true; + } + } + + return Constructors; +} + +MemberFunctionPairInfo getAssignmentsInfo(CXXRecordDecl const &Node) { + MemberFunctionPairInfo Assignments{}; + + for (CXXMethodDecl const *Method : Node.methods()) { + if (Method->isCopyAssignmentOperator()) { + Assignments.Copy.Declared = true; + if (Method->isDeleted()) + Assignments.Copy.Deleted = true; + } + + if (Method->isMoveAssignmentOperator()) { + Assignments.Move.Declared = true; + if (Method->isDeleted()) + Assignments.Move.Deleted = true; + } + } + + return Assignments; +} + +AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) { + MemberFunctionPairInfo Constructors = getConstructorsInfo(Node); + MemberFunctionPairInfo Assignments = getAssignmentsInfo(Node); + + if (Node.hasSimpleCopyConstructor() || + (Constructors.Copy.Declared && !Constructors.Copy.Deleted)) + return true; + if (Node.hasSimpleMoveConstructor() || + (Constructors.Move.Declared && !Constructors.Move.Deleted)) + return true; + if (Node.hasSimpleCopyAssignment() || + (Assignments.Copy.Declared && !Assignments.Copy.Deleted)) + return true; + if (Node.hasSimpleMoveAssignment() || + (Assignments.Move.Declared && !Assignments.Move.Deleted)) + return true; + + return false; +} + } // namespace void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()), - hasType(hasCanonicalType(referenceType()))) - .bind("ref"), - this); - Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()), - hasType(qualType(isConstQualified()))) - .bind("const"), - this); + Finder->addMatcher( + fieldDecl( + unless(isMemberOfLambda()), + anyOf( + fieldDecl(hasType(hasCanonicalType(referenceType()))).bind("ref"), + fieldDecl(hasType(qualType(isConstQualified()))).bind("const")), + hasDeclContext(cxxRecordDecl(isCopyableOrMovable()))), + this); } void AvoidConstOrRefDataMembersCheck::check( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1c542d4c9f2f3..1b8f3bf113c01 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -325,6 +325,11 @@ Changes in existing checks - Deprecated :doc:`cert-dcl21-cpp ` check. +- Fixed :doc:`cppcoreguidelines-avoid-const-or-ref-data-members + ` check + to emit warnings only on classes that are copyable/movable, as required by the + corresponding rule. + - Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer `. Please use :doc:`cppcoreguidelines-use-default-member-init diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst index b79872ad56d7d..773c51818e03d 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst @@ -3,9 +3,10 @@ cppcoreguidelines-avoid-const-or-ref-data-members ================================================= -This check warns when structs or classes have const-qualified or reference -(lvalue or rvalue) data members. Having such members is rarely useful, and -makes the class only copy-constructible but not copy-assignable. +This check warns when structs or classes that are copyable or movable, and have +const-qualified or reference (lvalue or rvalue) data members. Having such +members is rarely useful, and makes the class only copy-constructible but not +copy-assignable. Examples: diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp index becc3ee8ba9d8..5a5d05bb4e94e 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp @@ -205,3 +205,130 @@ void lambdas() auto c5 = x5; }; } + +struct NonCopyableWithRef +{ + NonCopyableWithRef(NonCopyableWithRef const&) = delete; + NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete; + NonCopyableWithRef(NonCopyableWithRef&&) = default; + NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default; + + int& x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference +}; + +struct NonMovableWithRef +{ + NonMovableWithRef(NonMovableWithRef const&) = default; + NonMovableWithRef& operator=(NonMovableWithRef const&) = default; + NonMovableWithRef(NonMovableWithRef&&) = delete; + NonMovableWithRef& operator=(NonMovableWithRef&&) = delete; + + int& x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference +}; + +struct NonCopyableNonMovableWithRef +{ + NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete; + NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete; + NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete; + NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete; + + int& x; // OK, non copyable nor movable +}; + +struct NonCopyable +{ + NonCopyable(NonCopyable const&) = delete; + NonCopyable& operator=(NonCopyable const&) = delete; + NonCopyable(NonCopyable&&) = default; + NonCopyable& operator=(NonCopyable&&) = default; +}; + +struct NonMovable +{ + NonMovable(NonMovable const&) = default; + NonMovable& operator=(NonMovable const&) = default; + NonMovable(NonMovable&&) = delete; + NonMovable& operator=(NonMovable&&) = delete; +}; + +struct NonCopyableNonMovable +{ + NonCopyableNonMovable(NonCopyableNonMovable const&) = delete; + NonCopyableNonMovable(NonCopyableNonMovable&&) = delete; + NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete; + NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete; +}; + +// Test inheritance +struct InheritFromNonCopyable : NonCopyable +{ + int& x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference +}; + +struct InheritFromNonMovable : NonMovable +{ + int& x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference +}; + +struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable +{ + int& x; // OK, non copyable nor movable +}; + +struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable +{ + int& x; // OK, non copyable nor movable +}; + +// Test composition +struct ContainsNonCopyable +{ + NonCopyable x; + int& y; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference +}; + +struct ContainsNonMovable +{ + NonMovable x; + int& y; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference +}; + +struct ContainsNonCopyableNonMovable +{ + NonCopyableNonMovable x; + int& y; // OK, non copyable nor movable +}; + +struct ContainsBothNonCopyableAndNonMovable +{ + NonCopyable x; + NonMovable y; + int& z; // OK, non copyable nor movable +}; + +// If copies are deleted and moves are not declared, moves are not implicitly declared, +// so the class is also not movable and we should not warn +struct NonCopyableMovesNotDeclared +{ + NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete; + NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete; + + int& x; // OK, non copyable nor movable +}; + +// If moves are deleted but copies are not declared, copies are implicitly deleted, +// so the class is also not copyable and we should not warn +struct NonMovableCopiesNotDeclared +{ + NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete; + NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete; + + int& x; // OK, non copyable nor movable +};