[clang-tools-extra] [clang-tidy] Fix false positive in cppcoreguidelines-avoid-const-or-ref-data-members when detecting templated classes with inheritance (PR #115180)

[Carlos Galvez via cfe-commits]

================
@@ -13,79 +13,88 @@
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
-namespace {
 
-AST_MATCHER(FieldDecl, isMemberOfLambda) {
-  return Node.getParent()->isLambda();
+static 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 && !hasCopyConstructor(*BRD))
+        return false;
+    }
+  }
+  if (Node.hasSimpleCopyConstructor())
+    return true;
+  for (CXXConstructorDecl const *Ctor : Node.ctors())
+    if (Ctor->isCopyConstructor())
+      return !Ctor->isDeleted();
+  return false;
 }
 
-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;
+static 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 && !hasMoveConstructor(*BRD))
+        return false;
     }
   }
-
-  return Constructors;
+  if (Node.hasSimpleMoveConstructor())
+    return true;
+  for (CXXConstructorDecl const *Ctor : Node.ctors())
+    if (Ctor->isMoveConstructor())
+      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;
+static 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 && !hasCopyAssignment(*BRD))
+        return false;
     }
+  }
+  if (Node.hasSimpleCopyAssignment())
+    return true;
+  for (CXXMethodDecl const *Method : Node.methods())
+    if (Method->isCopyAssignmentOperator())
+      return !Method->isDeleted();
+  return false;
+}
 
-    if (Method->isMoveAssignmentOperator()) {
-      Assignments.Move.Declared = true;
-      if (Method->isDeleted())
-        Assignments.Move.Deleted = true;
+static 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 && !hasMoveAssignment(*BRD))
+        return false;
     }
   }
-
-  return Assignments;
+  if (Node.hasSimpleMoveAssignment())
+    return true;
+  for (CXXMethodDecl const *Method : Node.methods())
+    if (Method->isMoveAssignmentOperator())
+      return !Method->isDeleted();
+  return false;
 }
 
-AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
-  MemberFunctionPairInfo Constructors = getConstructorsInfo(Node);
-  MemberFunctionPairInfo Assignments = getAssignmentsInfo(Node);
+namespace {
 
-  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;
+AST_MATCHER(FieldDecl, isMemberOfLambda) {
+  return Node.getParent()->isLambda();
+}
 
-  return false;
+AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
+  return hasCopyConstructor(Node) || hasMoveConstructor(Node) ||
----------------
carlosgalvezp wrote:

Nit, a class can "have" a copy constructor, but it can be deleted, so it 
doesn't help here.

I think it would be more accurate to say `return isCopyConstructible(Node) || 
...`

https://github.com/llvm/llvm-project/pull/115180
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits