[clang-tools-extra] b70e6e9 - [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members
Carlos Galvez via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 19 05:06:27 PDT 2023
Author: Carlos Galvez
Date: 2023-07-19T12:05:15Z
New Revision: b70e6e9681925ad06d9899462b9e43250be53f64
URL: https://github.com/llvm/llvm-project/commit/b70e6e9681925ad06d9899462b9e43250be53f64
DIFF: https://github.com/llvm/llvm-project/commit/b70e6e9681925ad06d9899462b9e43250be53f64.diff
LOG: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members
Since that's what the guidelines require.
Fixes #63733
Differential Revision: https://reviews.llvm.org/D155625
Added:
Modified:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index b22fa94663d584..6a6e620a4387b0 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 1c542d4c9f2f30..1b8f3bf113c01c 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
<clang-tidy/checks/cert/dcl21-cpp>` check.
+- Fixed :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+ <clang-tidy/checks/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
<clang-tidy/checks/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 b79872ad56d7dc..773c51818e03d3 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 becc3ee8ba9d82..5a5d05bb4e94ee 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
+};
More information about the cfe-commits
mailing list