carlosgalvezp updated this revision to Diff 541963.
carlosgalvezp added a comment.
Merge matchers.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155625/new/
https://reviews.llvm.org/D155625
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
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
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
+++ 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:
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -316,6 +316,11 @@
- 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
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -19,17 +19,86 @@
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(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
