[PATCH] D126891: [clang-tidy] The check should ignore final classes

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: aaron.ballman, njames93, LegalizeAdulthood, whisperity.
Herald added subscribers: carlosgalvezp, martong, rnkovacs, kbarton, xazax.hun, nemanjai.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126891

Files:
  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


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
@@ -320,3 +320,22 @@
 #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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -165,6 +165,12 @@
   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 `final` marked classes, 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.
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
@@ -41,6 +41,7 @@
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
+          unless(isFinal()),
           unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
           .bind("ProblematicClassOrStruct"),
       this);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D126891.433770.patch
Type: text/x-patch
Size: 2369 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220602/f7eb2252/attachment.bin>