https://github.com/HerrCai0907 updated
https://github.com/llvm/llvm-project/pull/115180
>From 428283c7b61ca50d40ffd3ddc5c08aca39f39533 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0...@163.com>
Date: Thu, 7 Nov 2024 00:35:47 +0800
Subject: [PATCH 1/4] [clang-tidy] fix false positive when detecting templated
classes with inheritance
`hasSimpleCopyConstructor` series of functions are not reliable when these
functions are not resolved. We need to manually resolve the status of these
functions from its base classes.
Fixes: #111985.
---
.../AvoidConstOrRefDataMembersCheck.cpp | 122 ++++++++++--------
clang-tools-extra/docs/ReleaseNotes.rst | 4 +
.../avoid-const-or-ref-data-members.cpp | 22 ++++
3 files changed, 93 insertions(+), 55 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index 6a6e620a4387b0..e1914eabd93f08 100644
---
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -19,75 +19,87 @@ 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;
+bool hasCopyConstructor(CXXRecordDecl const &Node) {
+ if (Node.needsOverloadResolutionForCopyConstructor() &&
+ Node.needsImplicitCopyConstructor()) {
+ // unresolved
+ for (CXXBaseSpecifier const &BS : Node.bases()) {
+ CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+ if (BRD != nullptr)
+ if (!hasCopyConstructor(*BRD))
+ return false;
}
}
-
- return Constructors;
+ if (Node.hasSimpleCopyConstructor())
+ return true;
+ for (CXXConstructorDecl const *Ctor : Node.ctors())
+ if (Ctor->isCopyConstructor())
+ return !Ctor->isDeleted();
+ return false;
}
-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;
+bool hasMoveConstructor(CXXRecordDecl const &Node) {
+ if (Node.needsOverloadResolutionForMoveConstructor() &&
+ Node.needsImplicitMoveConstructor()) {
+ // unresolved
+ for (CXXBaseSpecifier const &BS : Node.bases()) {
+ CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+ if (BRD != nullptr)
+ if (!hasMoveConstructor(*BRD))
+ return false;
}
+ }
+ if (Node.hasSimpleMoveConstructor())
+ return true;
+ for (CXXConstructorDecl const *Ctor : Node.ctors())
+ if (Ctor->isMoveConstructor())
+ return !Ctor->isDeleted();
+ return false;
+}
- if (Method->isMoveAssignmentOperator()) {
- Assignments.Move.Declared = true;
- if (Method->isDeleted())
- Assignments.Move.Deleted = true;
+bool hasCopyAssignment(CXXRecordDecl const &Node) {
+ if (Node.needsOverloadResolutionForCopyAssignment() &&
+ Node.needsImplicitCopyAssignment()) {
+ // unresolved
+ for (CXXBaseSpecifier const &BS : Node.bases()) {
+ CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+ if (BRD != nullptr)
+ if (!hasCopyAssignment(*BRD))
+ return false;
}
}
-
- return Assignments;
+ if (Node.hasSimpleCopyAssignment())
+ return true;
+ for (CXXMethodDecl const *Method : Node.methods())
+ if (Method->isCopyAssignmentOperator())
+ return !Method->isDeleted();
+ return false;
}
-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))
+bool hasMoveAssignment(CXXRecordDecl const &Node) {
+ if (Node.needsOverloadResolutionForMoveAssignment() &&
+ Node.needsImplicitMoveAssignment()) {
+ // unresolved
+ for (CXXBaseSpecifier const &BS : Node.bases()) {
+ CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+ if (BRD != nullptr)
+ if (!hasMoveAssignment(*BRD))
+ return false;
+ }
+ }
+ if (Node.hasSimpleMoveAssignment())
return true;
-
+ for (CXXMethodDecl const *Method : Node.methods())
+ if (Method->isMoveAssignmentOperator())
+ return !Method->isDeleted();
return false;
}
+AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
+ return hasCopyConstructor(Node) || hasMoveConstructor(Node) ||
+ hasCopyAssignment(Node) || hasMoveAssignment(Node);
+}
+
} // namespace
void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 51ba157ab05deb..f4cc32fbe50711 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -190,6 +190,10 @@ Changes in existing checks
fix false positive that floating point variable is only used in increment
expression.
+- Improved :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+ <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check
to
+ avoid false positive when detecting templated class with inheritance.
+
- Improved :doc:`cppcoreguidelines-prefer-member-initializer
<clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
avoid false positive when member initialization depends on a structured
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 e3864be134da3c..19da88300aec46 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
@@ -285,6 +285,28 @@ struct InheritBothFromNonCopyableAndNonMovable :
NonCopyable, NonMovable
int& x; // OK, non copyable nor movable
};
+template<class T> struct TemplateInheritFromNonCopyable : NonCopyable
+{
+ int& x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a
reference
+};
+
+template<class T> struct TemplateInheritFromNonMovable : NonMovable
+{
+ int& x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a
reference
+};
+
+template<class T> struct TemplateInheritFromNonCopyableNonMovable :
NonCopyableNonMovable
+{
+ int& x; // OK, non copyable nor movable
+};
+
+template<class T> struct TemplateInheritBothFromNonCopyableAndNonMovable :
NonCopyable, NonMovable
+{
+ int& x; // OK, non copyable nor movable
+};
+
// Test composition
struct ContainsNonCopyable
{
>From c2e0f7c55d991f6492e0ab8dc5214f95566d1bdf Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0...@163.com>
Date: Thu, 14 Nov 2024 09:27:28 +0800
Subject: [PATCH 2/4] fix review
---
.../AvoidConstOrRefDataMembersCheck.cpp | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index e1914eabd93f08..f577bd316b1b4b 100644
---
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -13,13 +13,8 @@
using namespace clang::ast_matchers;
namespace clang::tidy::cppcoreguidelines {
-namespace {
-
-AST_MATCHER(FieldDecl, isMemberOfLambda) {
- return Node.getParent()->isLambda();
-}
-bool hasCopyConstructor(CXXRecordDecl const &Node) {
+static bool hasCopyConstructor(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForCopyConstructor() &&
Node.needsImplicitCopyConstructor()) {
// unresolved
@@ -38,7 +33,7 @@ bool hasCopyConstructor(CXXRecordDecl const &Node) {
return false;
}
-bool hasMoveConstructor(CXXRecordDecl const &Node) {
+static bool hasMoveConstructor(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForMoveConstructor() &&
Node.needsImplicitMoveConstructor()) {
// unresolved
@@ -57,7 +52,7 @@ bool hasMoveConstructor(CXXRecordDecl const &Node) {
return false;
}
-bool hasCopyAssignment(CXXRecordDecl const &Node) {
+static bool hasCopyAssignment(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForCopyAssignment() &&
Node.needsImplicitCopyAssignment()) {
// unresolved
@@ -76,7 +71,7 @@ bool hasCopyAssignment(CXXRecordDecl const &Node) {
return false;
}
-bool hasMoveAssignment(CXXRecordDecl const &Node) {
+static bool hasMoveAssignment(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForMoveAssignment() &&
Node.needsImplicitMoveAssignment()) {
// unresolved
@@ -95,6 +90,12 @@ bool hasMoveAssignment(CXXRecordDecl const &Node) {
return false;
}
+namespace {
+
+AST_MATCHER(FieldDecl, isMemberOfLambda) {
+ return Node.getParent()->isLambda();
+}
+
AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
return hasCopyConstructor(Node) || hasMoveConstructor(Node) ||
hasCopyAssignment(Node) || hasMoveAssignment(Node);
>From deb6e54316fc07c46e453fc3536ab10f80fcf2ef Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0...@163.com>
Date: Mon, 18 Nov 2024 13:44:26 +0800
Subject: [PATCH 3/4] fix review
---
.../AvoidConstOrRefDataMembersCheck.cpp | 20 ++++++++-----------
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index f577bd316b1b4b..852db4ba8dc083 100644
---
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -20,9 +20,8 @@ static bool hasCopyConstructor(CXXRecordDecl const &Node) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr)
- if (!hasCopyConstructor(*BRD))
- return false;
+ if (BRD != nullptr && !hasCopyConstructor(*BRD))
+ return false;
}
}
if (Node.hasSimpleCopyConstructor())
@@ -39,9 +38,8 @@ static bool hasMoveConstructor(CXXRecordDecl const &Node) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr)
- if (!hasMoveConstructor(*BRD))
- return false;
+ if (BRD != nullptr && !hasMoveConstructor(*BRD))
+ return false;
}
}
if (Node.hasSimpleMoveConstructor())
@@ -58,9 +56,8 @@ static bool hasCopyAssignment(CXXRecordDecl const &Node) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr)
- if (!hasCopyAssignment(*BRD))
- return false;
+ if (BRD != nullptr && !hasCopyAssignment(*BRD))
+ return false;
}
}
if (Node.hasSimpleCopyAssignment())
@@ -77,9 +74,8 @@ static bool hasMoveAssignment(CXXRecordDecl const &Node) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr)
- if (!hasMoveAssignment(*BRD))
- return false;
+ if (BRD != nullptr && !hasMoveAssignment(*BRD))
+ return false;
}
}
if (Node.hasSimpleMoveAssignment())
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 0ab1ab85bd57b9..039ed658aada7f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -197,7 +197,7 @@ Changes in existing checks
- Improved :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
<clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check
to
- avoid false positive when detecting templated class with inheritance.
+ avoid false positives when detecting a templated class with inheritance.
- Improved :doc:`cppcoreguidelines-init-variables
<clang-tidy/checks/cppcoreguidelines/init-variables>` check by fixing the
>From 993db43df5132f6efa9e2e8cff5a7458abdbfc18 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0...@163.com>
Date: Sat, 23 Nov 2024 14:34:20 +0800
Subject: [PATCH 4/4] fix review
---
.../AvoidConstOrRefDataMembersCheck.cpp | 20 +++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index 852db4ba8dc083..f615976c7edb62 100644
---
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -14,13 +14,13 @@ using namespace clang::ast_matchers;
namespace clang::tidy::cppcoreguidelines {
-static bool hasCopyConstructor(CXXRecordDecl const &Node) {
+static bool isCopyConstructible(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForCopyConstructor() &&
Node.needsImplicitCopyConstructor()) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr && !hasCopyConstructor(*BRD))
+ if (BRD != nullptr && !isCopyConstructible(*BRD))
return false;
}
}
@@ -32,13 +32,13 @@ static bool hasCopyConstructor(CXXRecordDecl const &Node) {
return false;
}
-static bool hasMoveConstructor(CXXRecordDecl const &Node) {
+static bool isMoveConstructible(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForMoveConstructor() &&
Node.needsImplicitMoveConstructor()) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr && !hasMoveConstructor(*BRD))
+ if (BRD != nullptr && !isMoveConstructible(*BRD))
return false;
}
}
@@ -50,13 +50,13 @@ static bool hasMoveConstructor(CXXRecordDecl const &Node) {
return false;
}
-static bool hasCopyAssignment(CXXRecordDecl const &Node) {
+static bool isCopyAssignable(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForCopyAssignment() &&
Node.needsImplicitCopyAssignment()) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr && !hasCopyAssignment(*BRD))
+ if (BRD != nullptr && !isCopyAssignable(*BRD))
return false;
}
}
@@ -68,13 +68,13 @@ static bool hasCopyAssignment(CXXRecordDecl const &Node) {
return false;
}
-static bool hasMoveAssignment(CXXRecordDecl const &Node) {
+static bool isMoveAssignable(CXXRecordDecl const &Node) {
if (Node.needsOverloadResolutionForMoveAssignment() &&
Node.needsImplicitMoveAssignment()) {
// unresolved
for (CXXBaseSpecifier const &BS : Node.bases()) {
CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
- if (BRD != nullptr && !hasMoveAssignment(*BRD))
+ if (BRD != nullptr && !isMoveAssignable(*BRD))
return false;
}
}
@@ -93,8 +93,8 @@ AST_MATCHER(FieldDecl, isMemberOfLambda) {
}
AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
- return hasCopyConstructor(Node) || hasMoveConstructor(Node) ||
- hasCopyAssignment(Node) || hasMoveAssignment(Node);
+ return isCopyConstructible(Node) || isMoveConstructible(Node) ||
+ isCopyAssignable(Node) || isMoveAssignable(Node);
}
} // namespace
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
