[clang-tools-extra] d9afb8c - [clang-tidy] cppcoreguidelines-virtual-class-destructor should ignore final classes

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2022-06-21T11:02:18+02:00
New Revision: d9afb8c3e8fd01a3c89ab2ddebcd44602a30a975

URL: https://github.com/llvm/llvm-project/commit/d9afb8c3e8fd01a3c89ab2ddebcd44602a30a975
DIFF: https://github.com/llvm/llvm-project/commit/d9afb8c3e8fd01a3c89ab2ddebcd44602a30a975.diff

LOG: [clang-tidy] cppcoreguidelines-virtual-class-destructor should ignore final classes

The `cppcoreguidelines-virtual-class-destructor` supposed to enforce
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual

Quote:
> A **base** class destructor should be either public and virtual, or
> protected and non-virtual
[emphasis mine]

However, this check still rules the following case:

  class MostDerived final : public Base {
  public:
    MostDerived() = default;
    ~MostDerived() = default;
    void func() final;
  };

Even though `MostDerived` class is marked `final`, thus it should not be
considered as a **base** class. Consequently, the rule is satisfied, yet
the check still flags this code.

In this patch, I'm proposing to ignore `final` classes since they cannot
be //base// classes.

Reviewed By: whisperity

Differential Revision:  https://reviews.llvm.org/D126891

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
index 26ee7ca6eca57..e730fb6fa01db 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -41,6 +41,7 @@ void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
+          unless(isFinal()),
           unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
           .bind("ProblematicClassOrStruct"),
       this);

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 36cac762f0d31..26d99420d3e4b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -165,6 +165,12 @@ Changes in existing checks
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
+- Fixed a false positive in :doc:`cppcoreguidelines-virtual-class-destructor
+  <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` involving
+  ``final`` classes. The check will not diagnose classes marked ``final``, since
+  those cannot be used as base classes, consequently, they can not violate the
+  rule.
+
 - Fixed a crash in :doc:`llvmlibc-callee-namespace
   <clang-tidy/checks/llvmlibc/callee-namespace>` when executing for C++ code
   that contain calls to advanced constructs, e.g. overloaded operators.

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 c14a2e68def88..fcf558dcac8e6 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
@@ -320,3 +320,22 @@ class FooBar5 {
 #undef XMACRO
 #undef CONCAT
 } // namespace macro_tests
+
+namespace FinalClassCannotBeBaseClass {
+class Base {
+public:
+  Base() = default;
+  virtual void func() = 0;
+
+protected:
+  ~Base() = default;
+};
+
+// no-warning: 'MostDerived' cannot be a base class, since it's marked 'final'.
+class MostDerived final : public Base {
+public:
+  MostDerived() = default;
+  ~MostDerived() = default;
+  void func() final;
+};
+} // namespace FinalClassCannotBeBaseClass