https://github.com/daniel-petrovic created https://github.com/llvm/llvm-project/pull/227198
Fixes #224686 >From 07cb446bb94056f94e383d8898303e1e185e7119 Mon Sep 17 00:00:00 2001 From: Daniel Petrovic <[email protected]> Date: Tue, 29 Sep 2026 08:21:11 +0200 Subject: [PATCH] [clang-tidy] Report virtual-class-destructor at the destructor location Fixes #224686 --- .../VirtualClassDestructorCheck.cpp | 20 +++--- clang-tools-extra/docs/ReleaseNotes.md | 7 +++ .../virtual-class-destructor.cpp | 62 +++++++++---------- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp index e43a652ed9d0c..3b5564e3ae9a7 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp @@ -174,15 +174,21 @@ void VirtualClassDestructorCheck::check( if (!Destructor) return; + const bool HasUserDeclaredDtor = + MatchedClassOrStruct->hasUserDeclaredDestructor(); + + const SourceLocation DiagLoc = HasUserDeclaredDtor + ? Destructor->getLocation() + : MatchedClassOrStruct->getLocation(); + if (Destructor->getAccess() == AccessSpecifier::AS_private) { - diag(MatchedClassOrStruct->getLocation(), - "destructor of %0 is private and prevents using the type") + diag(DiagLoc, "destructor of %0 is private and prevents using the type") << MatchedClassOrStruct; - diag(MatchedClassOrStruct->getLocation(), + diag(DiagLoc, /*Description=*/"make it public and virtual", DiagnosticIDs::Note) << changePrivateDestructorVisibilityTo( "public", *Destructor, *Result.SourceManager, getLangOpts()); - diag(MatchedClassOrStruct->getLocation(), + diag(DiagLoc, /*Description=*/"make it protected", DiagnosticIDs::Note) << changePrivateDestructorVisibilityTo( "protected", *Destructor, *Result.SourceManager, getLangOpts()); @@ -194,7 +200,7 @@ void VirtualClassDestructorCheck::check( bool ProtectedAndVirtual = false; FixItHint Fix; - if (MatchedClassOrStruct->hasUserDeclaredDestructor()) { + if (HasUserDeclaredDtor) { if (Destructor->getAccess() == AccessSpecifier::AS_public) { Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual "); } else if (Destructor->getAccess() == AccessSpecifier::AS_protected) { @@ -209,11 +215,11 @@ void VirtualClassDestructorCheck::check( *Result.SourceManager); } - diag(MatchedClassOrStruct->getLocation(), + diag(DiagLoc, "destructor of %0 is %select{public and non-virtual|protected and " "virtual}1") << MatchedClassOrStruct << ProtectedAndVirtual; - diag(MatchedClassOrStruct->getLocation(), + diag(DiagLoc, "make it %select{public and virtual|protected and non-virtual}0", DiagnosticIDs::Note) << ProtectedAndVirtual << Fix; diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 833638a47abc6..3eb7ea7b4c0ba 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -200,6 +200,13 @@ infrastructure are described first, followed by tool-specific sections. - Improved {doc}`cppcoreguidelines-use-enum-class <clang-tidy/checks/cppcoreguidelines/use-enum-class>` check by omitting unnamed enums from the `enum class` requirement, as previously the check suggested users an ill-formed fix. +- Improved {doc}`cppcoreguidelines-virtual-class-destructor + <clang-tidy/checks/cppcoreguidelines/virtual-class-destructor>` check by + emitting the diagnostic and its fix-it notes at the destructor's location + instead of the class name, whenever the destructor is user-declared. The + diagnostics are still emitted at the class name for implicitly declared + destructors. + - Improved {doc}`misc-const-correctness <clang-tidy/checks/misc/const-correctness>` check: diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/virtual-class-destructor.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/virtual-class-destructor.cpp index 725a7094a0f17..9cfde2e02b3d0 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/virtual-class-destructor.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/virtual-class-destructor.cpp @@ -1,8 +1,8 @@ // RUN: %check_clang_tidy %s cppcoreguidelines-virtual-class-destructor %t -- --fix-notes -// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PrivateVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual -// CHECK-MESSAGES: :[[@LINE+2]]:8: note: make it protected +// CHECK-MESSAGES: :[[@LINE+8]]:11: warning: destructor of 'PrivateVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+7]]:11: note: make it public and virtual +// CHECK-MESSAGES: :[[@LINE+6]]:11: note: make it protected // As we have 2 conflicting fixes in notes, no fix is applied. struct PrivateVirtualBaseStruct { virtual void f(); @@ -16,8 +16,8 @@ struct PublicVirtualBaseStruct { // OK virtual ~PublicVirtualBaseStruct() {} }; -// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'ProtectedVirtualBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+6]]:11: warning: destructor of 'ProtectedVirtualBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+5]]:11: note: make it protected and non-virtual struct ProtectedVirtualBaseStruct { virtual void f(); @@ -26,8 +26,8 @@ struct ProtectedVirtualBaseStruct { // CHECK-FIXES: ~ProtectedVirtualBaseStruct() {} }; -// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'ProtectedVirtualDefaultBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+6]]:11: warning: destructor of 'ProtectedVirtualDefaultBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+5]]:11: note: make it protected and non-virtual struct ProtectedVirtualDefaultBaseStruct { virtual void f(); @@ -36,9 +36,9 @@ struct ProtectedVirtualDefaultBaseStruct { // CHECK-FIXES: ~ProtectedVirtualDefaultBaseStruct() = default; }; -// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PrivateNonVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual -// CHECK-MESSAGES: :[[@LINE+2]]:8: note: make it protected +// CHECK-MESSAGES: :[[@LINE+8]]:3: warning: destructor of 'PrivateNonVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+7]]:3: note: make it public and virtual +// CHECK-MESSAGES: :[[@LINE+6]]:3: note: make it protected // As we have 2 conflicting fixes in notes, no fix is applied. struct PrivateNonVirtualBaseStruct { virtual void f(); @@ -47,8 +47,8 @@ struct PrivateNonVirtualBaseStruct { ~PrivateNonVirtualBaseStruct() {} }; -// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'PublicNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it public and virtual +// CHECK-MESSAGES: :[[@LINE+4]]:3: warning: destructor of 'PublicNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:3: note: make it public and virtual struct PublicNonVirtualBaseStruct { virtual void f(); ~PublicNonVirtualBaseStruct() {} @@ -85,9 +85,9 @@ struct ProtectedNonVirtualBaseStruct { // OK ~ProtectedNonVirtualBaseStruct() {} }; -// CHECK-MESSAGES: :[[@LINE+4]]:7: warning: destructor of 'PrivateVirtualBaseClass' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+3]]:7: note: make it public and virtual -// CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected +// CHECK-MESSAGES: :[[@LINE+6]]:11: warning: destructor of 'PrivateVirtualBaseClass' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+5]]:11: note: make it public and virtual +// CHECK-MESSAGES: :[[@LINE+4]]:11: note: make it protected // As we have 2 conflicting fixes in notes, no fix is applied. class PrivateVirtualBaseClass { virtual void f(); @@ -101,8 +101,8 @@ class PublicVirtualBaseClass { // OK virtual ~PublicVirtualBaseClass() {} }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'ProtectedVirtualBaseClass' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+6]]:11: warning: destructor of 'ProtectedVirtualBaseClass' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+5]]:11: note: make it protected and non-virtual class ProtectedVirtualBaseClass { virtual void f(); @@ -133,8 +133,8 @@ class PublicASImplicitNonVirtualBaseClass { int foo = 42; }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'PublicNonVirtualBaseClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it public and virtual +// CHECK-MESSAGES: :[[@LINE+6]]:3: warning: destructor of 'PublicNonVirtualBaseClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+5]]:3: note: make it public and virtual class PublicNonVirtualBaseClass { virtual void f(); @@ -275,44 +275,44 @@ namespace macro_tests { #define MY_VIRTUAL virtual #define CONCAT(x, y) x##y -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+4]]:28: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:28: note: make it protected and non-virtual class FooBar1 { protected: CONCAT(vir, tual) CONCAT(~Foo, Bar1()); // no-fixit }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar2' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+4]]:18: warning: destructor of 'FooBar2' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:18: note: make it protected and non-virtual class FooBar2 { protected: virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this. }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar3' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+4]]:21: warning: destructor of 'FooBar3' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:21: note: make it protected and non-virtual class FooBar3 { protected: CONCAT(vir, tual) ~FooBar3(); // FIXME: We should have a fixit for this. }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar4' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+4]]:21: warning: destructor of 'FooBar4' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:21: note: make it protected and non-virtual class FooBar4 { protected: CONCAT(vir, tual) ~CONCAT(Foo, Bar4()); // FIXME: We should have a fixit for this. }; -// CHECK-MESSAGES: :[[@LINE+3]]:7: warning: destructor of 'FooBar5' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+5]]:29: warning: destructor of 'FooBar5' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+4]]:29: note: make it protected and non-virtual #define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2 class FooBar5 { protected: XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit }; -// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar6' is protected and virtual [cppcoreguidelines-virtual-class-destructor] -// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual +// CHECK-MESSAGES: :[[@LINE+4]]:14: warning: destructor of 'FooBar6' is protected and virtual [cppcoreguidelines-virtual-class-destructor] +// CHECK-MESSAGES: :[[@LINE+3]]:14: note: make it protected and non-virtual class FooBar6 { protected: MY_VIRTUAL ~FooBar6(); // FIXME: We should have a fixit for this. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
