[clang-tidy] The check should ignore final classes

[steakhal]

Summary

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.

[carlosgalvezp]

Hmm, MostDerived does have a public virtual destructor in your example already - if the base class destructor is virtual, the child class destructor is virtual. In that sense the check should not warn.

Seems like there's some deeper problem in the check?

[steakhal]

In D126891#3554039, @carlosgalvezp wrote:

Hmm, MostDerived does have a public virtual destructor in your example already - if the base class destructor is virtual, the child class destructor is virtual. In that sense the check should not warn.

Seems like there's some deeper problem in the check?

Not quite. None of the destructors are virtual in the example. In fact, the MostDerived has a public and non-virtual destructor.

[carlosgalvezp]

In D126891#3555456, @steakhal wrote:

In D126891#3554039, @carlosgalvezp wrote:

Hmm, MostDerived does have a public virtual destructor in your example already - if the base class destructor is virtual, the child class destructor is virtual. In that sense the check should not warn.

Seems like there's some deeper problem in the check?

Not quite. None of the destructors are virtual in the example. In fact, the MostDerived has a public and non-virtual destructor.

Doesn't Base have a virtual destructor? As per the standard:

If a class has a base class with a virtual destructor, its destructor (whether user- or implicitly-declared) is virtual.

To clarify: I'm not opposed to the patch, I'm just trying to understand if the AST is indeed implementing the Standard correctly and classifying the Derived destructor as virtual.

[carlosgalvezp]

Oh, I see the unit test now, indeed Base does not have a virtual destructor. LGTM then!

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

170

I believe the convention is to use double-backtick for C++ keywords and functions, see line 160.

[steakhal]

clang-tools-extra/docs/ReleaseNotes.rst

170

Thanks, I'll fix it. I should learn rst for once.

[steakhal]

• rebase
• use double-backticks for the keyword final in the Release Notes.

polite ping

[steakhal]

ping again; @whisperity

[steakhal]

@aaron.ballman @njames93

[whisperity]

Thank you!

clang-tools-extra/docs/ReleaseNotes.rst

170

(Nit: classes marked final)

[whisperity]

(Please ensure a more appropriate commit message that actually mentions the check when committing, @steakhal.)

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

 
 void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
       hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
 
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
+          unless(isFinal()),
           unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
           .bind("ProblematicClassOrStruct"),
       this);
 }
 
 static Optional<CharSourceRange>
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
                        const SourceManager &SM, const LangOptions &LangOpts) {

clang-tools-extra/docs/ReleaseNotes.rst

- Made :doc:`cert-oop57-cpp <clang-tidy/checks/cert/oop57-cpp>` more sensitive
   by checking for an arbitrary expression in the second argument of ``memset``.
 
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check.
 
   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.
 
 - Fixed a false positive in :doc:`misc-redundant-expression
   <clang-tidy/checks/misc/redundant-expression>` involving overloaded
   comparison operators.
 

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

 #define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2
 class FooBar5 {
 protected:
   XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
 };
 #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