carlosgalvezp updated this revision to Diff 376168.
carlosgalvezp added a comment.
Created custom matcher and moved the logic from check to match stage.
Documented why we can't simply match a CXXDestructorDecl.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110614/new/
https://reviews.llvm.org/D110614
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
Index:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
---
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
void m();
};
// inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of
'ForwardDeclaredStruct' is public and non-virtual
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of
'TemplatedDerived' is public and non-virtual
[cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived<int> InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of
'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual
[cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct DerivedFromTemplateVirtualBaseStruct : T {
+ virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct<PublicVirtualBaseStruct>
InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of
'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual
[cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of
'DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct>' is public
and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() =
default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template <typename T>
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+ virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct>
InstantiationWithPublicNonVirtualBaseStruct;
+
+} // namespace Bugzilla_51912
Index:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
---
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -19,6 +19,21 @@
namespace tidy {
namespace cppcoreguidelines {
+AST_MATCHER(CXXRecordDecl, hasPublicVirtualOrProtectedNonVirtualDestructor) {
+ // We need to call Node.getDestructor() instead of matching a
+ // CXXDestructorDecl. Otherwise, tests will fail for class templates, since
+ // the primary template (not the specialization) always gets a non-virtual
+ // CXXDestructorDecl in the AST. https://bugs.llvm.org/show_bug.cgi?id=51912
+ const CXXDestructorDecl *Destructor = Node.getDestructor();
+ if (!Destructor)
+ return false;
+
+ return (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+ Destructor->isVirtual()) ||
+ ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+ !Destructor->isVirtual()));
+}
+
void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
@@ -26,9 +41,7 @@
Finder->addMatcher(
cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
- unless(anyOf(
- has(cxxDestructorDecl(isPublic(), isVirtual())),
- has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
+ unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
.bind("ProblematicClassOrStruct"),
this);
}
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
void m();
};
// inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived<int> InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct DerivedFromTemplateVirtualBaseStruct : T {
+ virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct<PublicVirtualBaseStruct> InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct>' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template <typename T>
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+ virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct> InstantiationWithPublicNonVirtualBaseStruct;
+
+} // namespace Bugzilla_51912
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -19,6 +19,21 @@
namespace tidy {
namespace cppcoreguidelines {
+AST_MATCHER(CXXRecordDecl, hasPublicVirtualOrProtectedNonVirtualDestructor) {
+ // We need to call Node.getDestructor() instead of matching a
+ // CXXDestructorDecl. Otherwise, tests will fail for class templates, since
+ // the primary template (not the specialization) always gets a non-virtual
+ // CXXDestructorDecl in the AST. https://bugs.llvm.org/show_bug.cgi?id=51912
+ const CXXDestructorDecl *Destructor = Node.getDestructor();
+ if (!Destructor)
+ return false;
+
+ return (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+ Destructor->isVirtual()) ||
+ ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+ !Destructor->isVirtual()));
+}
+
void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
@@ -26,9 +41,7 @@
Finder->addMatcher(
cxxRecordDecl(
anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
- unless(anyOf(
- has(cxxDestructorDecl(isPublic(), isVirtual())),
- has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
+ unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
.bind("ProblematicClassOrStruct"),
this);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
