carlosgalvezp updated this revision to Diff 376139.
carlosgalvezp added a comment.
Moved the condition of public-virtual / protected-non-virtual to the check
stage instead of the match stage. This way is more similar to the equivalent
Sema check and it passes all the tests. Let me know what you think!
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
@@ -23,14 +23,10 @@
ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
- Finder->addMatcher(
- cxxRecordDecl(
- anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
- unless(anyOf(
- has(cxxDestructorDecl(isPublic(), isVirtual())),
- has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
- .bind("ProblematicClassOrStruct"),
- this);
+ Finder->addMatcher(cxxRecordDecl(anyOf(has(cxxMethodDecl(isVirtual())),
+ InheritsVirtualMethod))
+ .bind("ProblematicClassOrStruct"),
+ this);
}
static CharSourceRange
@@ -152,6 +148,12 @@
if (!Destructor)
return;
+ if (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+ Destructor->isVirtual()) ||
+ ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+ !Destructor->isVirtual()))
+ return;
+
if (Destructor->getAccess() == AccessSpecifier::AS_private) {
diag(MatchedClassOrStruct->getLocation(),
"destructor of %0 is private and prevents using the type")
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
@@ -23,14 +23,10 @@
ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
- Finder->addMatcher(
- cxxRecordDecl(
- anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
- unless(anyOf(
- has(cxxDestructorDecl(isPublic(), isVirtual())),
- has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
- .bind("ProblematicClassOrStruct"),
- this);
+ Finder->addMatcher(cxxRecordDecl(anyOf(has(cxxMethodDecl(isVirtual())),
+ InheritsVirtualMethod))
+ .bind("ProblematicClassOrStruct"),
+ this);
}
static CharSourceRange
@@ -152,6 +148,12 @@
if (!Destructor)
return;
+ if (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+ Destructor->isVirtual()) ||
+ ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+ !Destructor->isVirtual()))
+ return;
+
if (Destructor->getAccess() == AccessSpecifier::AS_private) {
diag(MatchedClassOrStruct->getLocation(),
"destructor of %0 is private and prevents using the type")
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
