[clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

[mgartmann]

Summary

Finds base classes and structs whose destructor is neither public and virtual nor protected and non-virtual.
A base class's destructor should be specified in one of these ways to prevent undefined behaviour.

Fixes are available for user-declared and implicit destructors that are either public
and non-virtual or protected and virtual.

This check implements C.35 from the CppCoreGuidelines.

[Eugene.Zelenko]

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

38 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

77 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

78 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

83 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

96 ↗

(On Diff #344806)

Why not just std::string DestructorString?

101 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

105 ↗

(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst

9 ↗

(On Diff #344806)

Links to documentation usually placed at the end. Please also try to follow 80 characters line length limit

16 ↗

(On Diff #344806)

Is it really necessary? Same below.

55 ↗

(On Diff #344806)

Shouldn't Clang-format take care about such things?

[njames93]

Whats the intended behaviour for derived classes and their destructors? Can test be added to demonstrate that behaviour?

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

44 ↗

(On Diff #344806)

Printing class or struct here isn't necessary, but if you really feel it should be included use %select{class|struct}0 instead.

51 ↗

(On Diff #344806)

This can no be made a bool, see below for the Diag.

68 ↗

(On Diff #344806)

70 ↗

(On Diff #344806)

Again this can be removed.

74 ↗

(On Diff #344806)

This doesn't need to be part of the class and should just be a static function in this file.

101 ↗

(On Diff #344806)

Again, don't worry about indentation, clang-format does that.

129 ↗

(On Diff #344806)

Can be made static again. Also should probably return a const AccessSpecDecl *

143–145 ↗

(On Diff #344806)

As clang-format handles indentation, we can just remove this function.

[mgartmann]

In D102325#2754241, @njames93 wrote:

Whats the intended behaviour for derived classes and their destructors? Can test be added to demonstrate that behaviour?

If a derived class inherits a virtual method from a base class, according to guideline C.35, it is a potential base class as well.
Therefore, such a derived class gets flagged by the check if its destructor is not specified correctly.

In order to flag derived classes that only inherit a virtual method but do not override it, the following matcher had to be added:

ast_matchers::internal::Matcher<CXXRecordDecl> inheritsVirtualMethod =
    hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));

Test cases were added to demonstrate this behaviour.

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

129 ↗

(On Diff #344806)

@njames93 If I get this correctly, only check and addMatcher should be member functions of a check's class. All the other aid methods should be static and not part of the class.

Did I get this right?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst

55 ↗

(On Diff #344806)

As @njames93 also suggested leaving the indentation to clang-format, I removed this option and the indentation functionality.

[mgartmann]

Incorporated Phabricator review feedback:

• added matchers and tests for subclasses with inherited virtual methods
• made aid methods static and not part of the check's class
• replaced auto with types where it was suggested
• adjusted diagnostic messages

Included new commits of upstream main branch to resolve build failure.

[Eugene.Zelenko]

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

22 ↗

(On Diff #345623)

Please fix readability-identifier-naming warning.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst

16 ↗

(On Diff #345623)

Links to documentation usually placed at the end. See other checks documentation.

[mgartmann]

Resolved readability-identifier-naming warning, adjusted check's documentation.

[mgartmann]

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst

16 ↗

(On Diff #345623)

Thanks for pointing this out to me!
I previously followed the style of other cppcoreguideline checks which did not have this line at the end of the file (e.g., cppcoreguidelines-prefer-member-initializer or cppcoreguidelines-avoid-non-const-global-variables.

I adjusted the documentation according to your suggestion and now placed this line at the end.

[mgartmann]

Dear reviewers,

Since this work was conducted as part of a Bachelor's thesis, which has to be
handed in on the 18th of June, 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Fabian & Marco

[aaron.ballman]

FWIW, this looks like it's also partially covering -Wnon-virtual-dtor, but that doesn't seem to have the checks for a protected destructor.

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

110–111 ↗

(On Diff #345635)

clang-tidy diagnostics should not contain full sentences with punctuation in them (they follow the Clang diagnostic rules).

One thing you could do to resolve this is to issue one diagnostic for the warning, and two diagnostics for notes with fixits attached to them. e.g.,

warning: destructor of %0 is 'private' and prevents using the type
note 1: consider making it public and virtual (fixit to insert public: before the declaration, insert virtual in the declaration,
and insert private: after the declaration)
note 2: consider making it protected (fixit to insert protected: before the declaration and insert private: after the declaration)

If you don't want to do the fixits, you could reword the diagnostic to combine the sentences, but it's a bit lengthy.

135–137 ↗

(On Diff #345635)

Same issue here as above.

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

8 ↗

(On Diff #345635)

All of these empty dtors have a semi-colon after their closing brace that can be removed.

I'd also like to see a test that = default works fine for the dtor.

[mgartmann]

• added fixes for private destructors
• separated fixes for private destructors into notes
• added a test case for a = default; constructor

[aaron.ballman]

Have you tried running this over any large C++ code bases to see how often the diagnostics trigger and whether there are false positives? If not, you should probably do so -- one concern I have is with a private virtual destructor; I could imagine people using that for a class that is intended to be created but not destroyed (like a singleton).

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

99

I think this may be a bit confusing of a name for the check -- this suggests to me it's about virtual base classes and their destructors, but the check is really more about defining a destructor properly in a class used as a base class with a vtable. However, this check follows the enforcement from the rule rather than the rule wording itself -- it doesn't care whether the class is ever used as a base class or not, it only cares whether the class contains a virtual function. How about: cppcoreguidelines-virtual-class-destructor? (Probably worth it to rename the check at the same time to keep the public check name and the internal check implementation names consistent.)

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

28 ↗

(On Diff #348988)

I believe you can remove this without changing the behavior -- unions can't have virtual member functions anyway.

80 ↗

(On Diff #348988)

89–93 ↗

(On Diff #348988)

This should be using an llvm::Twine (https://llvm.org/docs/ProgrammersManual.html#the-twine-class).

117–121 ↗

(On Diff #348988)

This function should also be using a Twine for much of this -- basically, Twine helps you build up a string from various components (some std::string, some StringRef, some const char *, etc) in an efficient manner that only does memory allocation when needing the full string contents.

123–125 ↗

(On Diff #348988)

135–141 ↗

(On Diff #348988)

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst

6–8 ↗

(On Diff #348988)

24–50 ↗

(On Diff #348988)

There are a bunch of trailing semicolons in the example code that can be removed.

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

6 ↗

(On Diff #348988)

Thank you for the comment about the fix not being applied!

[mgartmann]

• changed name of check to cppcoreguidelines-virtual-class-destructor
• removed matcher for class or struct in AST matcher
• changed string concatenations to use llvm::twine
• adjusted documentation and removed unnecessary semicolons in it

[mgartmann]

Thanks a lot for your feedback, @aaron.ballman!

I was able to incorporate it in the updated diff.

On your advice, I also ran this check on a large open source project using the run-clang-tidy.py script. I decided to do it on the llvm project itself as follows:

marco@nb:~/llvm-project/build$ python3 run-clang-tidy.py -checks="-*,cppcoreguidelines-virtual-class-destructor"  -header-filter=".*" > result.txt
marco@nb:~/llvm-project/build$ cat result.txt | grep "private and prevents" | wc -l
797

Most of these 797 lines are duplicates, since these warnings come from header files included in (apparently) many other files.
Deduplicated, the following seven destructors were private and triggered this check's warning:

• llvm-project/llvm/include/llvm/Analysis/RegionInfo.h:1024:23: warning: destructor of 'RegionInfoBase<llvm::RegionTraits<llvm::Function>>' is private ...
• llvm-project/llvm/include/llvm/Analysis/RegionInfo.h:674:7: warning: destructor of 'RegionInfoBase' is private ...
• llvm-project/llvm/include/llvm/CodeGen/MachineRegionInfo.h:177:23: warning: destructor of 'RegionInfoBase<llvm::RegionTraits<llvm::MachineFunction>>' is private ...
• llvm-project/llvm/lib/Target/AVR/MCTargetDesc/AVRMCExpr.h:19:7: warning: destructor of 'AVRMCExpr' is private ...
• llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1257:18: warning: destructor of 'UnitTest' is private ...
• llvm-project/llvm/lib/CodeGen/InlineSpiller.cpp:159:7: warning: destructor of 'InlineSpiller' is private ...
• llvm-project/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:387:7: warning: destructor of 'X86MCInstrAnalysis' is private ...

I assume that these destructors are private by on purpose. Please correct me if I am wrong.
A programmer working on the LLVM project would therefore see seven warnings for private destructors over the whole project.

Is this number of private destructors which are flagged acceptable to you?

[mgartmann]

• Updated this patch with the current state of the LLVM project GitHub repository

[aaron.ballman]

In D102325#2816919, @mgartmann wrote:

I assume that these destructors are private by on purpose. Please correct me if I am wrong.

As best I can tell, these look purposeful to me.

A programmer working on the LLVM project would therefore see seven warnings for private destructors over the whole project.

Wellllll not quite; because these are in header files, they'd see a lot more than just seven warnings.

Is this number of private destructors which are flagged acceptable to you?

To be honest, I think this signals that we wouldn't be able to enable this guideline for our codebase because it's too chatty with no value. Based on the results, it's only pointing out false positives and no true positives. So from that perspective, it's not super acceptable. However, I've come to the conclusion that automated checking for the C++ Core Guidelines (in general, not just with your patch!) is not particularly useful for existing code bases because of the lack of consideration put into existing code by the guideline authors. From that perspective, I think this check is fine.

LGTM, thank you for your patience while I pondered this!

[mgartmann]

@aaron.ballman Thanks a lot for your review!

Can somebody help me merging this change? I do not have the rights to do so.
Thanks in advanve!

[whisperity]

In D102325#2970683, @mgartmann wrote:

@aaron.ballman Thanks a lot for your review!

Can somebody help me merging this change? I do not have the rights to do so.
Thanks in advanve!

Sure, just please specify what name and e-mail address you'd like to have associated with the commit. Ever since we moved to Git, now we have a "built-in" way of distinguishing the two fields.

[mgartmann]

@whisperity Thanks a lot for helping me out!
Name: Marco Gartmann
E-Mail: [e-mail]

[whisperity]

In D102325#2981760, @mgartmann wrote:

@whisperity Thanks a lot for helping me out!

Sorry I got busy with a few official business and then got deeply distracted by what I'm working on, but I'll get to committing this ASAP. ETA 1 hr. 🙂

[lebedev.ri]

Question: should this really diagnose classes that are marked as final?
Surely such a class can never be a base class?

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

add_clang_library(clangTidyCppCoreGuidelinesModule
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
   SlicingCheck.cpp
   SpecialMemberFunctionsCheck.cpp
+  VirtualClassDestructorCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyReadabilityModule
   clangTidyUtils
 

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

 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 #include "SlicingCheck.h"
 #include "SpecialMemberFunctionsCheck.h"
+#include "VirtualClassDestructorCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
     CheckFactories.registerCheck<SpecialMemberFunctionsCheck>(
         "cppcoreguidelines-special-member-functions");
     CheckFactories.registerCheck<SlicingCheck>("cppcoreguidelines-slicing");
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
+    CheckFactories.registerCheck<VirtualClassDestructorCheck>(
+        "cppcoreguidelines-virtual-class-destructor");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
 
     Opts["cppcoreguidelines-non-private-member-variables-in-classes."
          "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "true";

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

+//===--- VirtualClassDestructorCheck.h - clang-tidy -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_VIRTUALCLASSDESTRUCTORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_VIRTUALCLASSDESTRUCTORCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <string>
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Finds base classes whose destructor is neither public and virtual
+/// nor protected and non-virtual.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-virtual-class-destructor.html
+class VirtualClassDestructorCheck : public ClangTidyCheck {
+public:
+  VirtualClassDestructorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_VIRTUALCLASSDESTRUCTORCHECK_H

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

+//===--- VirtualClassDestructorCheck.cpp - clang-tidy -----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "VirtualClassDestructorCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+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(anyOf(
+              has(cxxDestructorDecl(isPublic(), isVirtual())),
+              has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
+          .bind("ProblematicClassOrStruct"),
+      this);
+}
+
+static CharSourceRange
+getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
+                       const SourceManager &SM, const LangOptions &LangOpts) {
+  SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
+  SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
+      Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
+
+  /// Range ends with \c StartOfNextToken so that any whitespace after \c
+  /// virtual is included.
+  SourceLocation StartOfNextToken =
+      Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
+          .getValue()
+          .getLocation();
+
+  return CharSourceRange::getCharRange(VirtualBeginLoc, StartOfNextToken);
+}
+
+static const AccessSpecDecl *
+getPublicASDecl(const CXXRecordDecl &StructOrClass) {
+  for (DeclContext::specific_decl_iterator<AccessSpecDecl>
+           AS{StructOrClass.decls_begin()},
+       ASEnd{StructOrClass.decls_end()};
+       AS != ASEnd; ++AS) {
+    AccessSpecDecl *ASDecl = *AS;
+    if (ASDecl->getAccess() == AccessSpecifier::AS_public)
+      return ASDecl;
+  }
+
+  return nullptr;
+}
+
+static FixItHint
+generateUserDeclaredDestructor(const CXXRecordDecl &StructOrClass,
+                               const SourceManager &SourceManager) {
+  std::string DestructorString;
+  SourceLocation Loc;
+  bool AppendLineBreak = false;
+
+  const AccessSpecDecl *AccessSpecDecl = getPublicASDecl(StructOrClass);
+
+  if (!AccessSpecDecl) {
+    if (StructOrClass.isClass()) {
+      Loc = StructOrClass.getEndLoc();
+      DestructorString = "public:";
+      AppendLineBreak = true;
+    } else {
+      Loc = StructOrClass.getBraceRange().getBegin().getLocWithOffset(1);
+    }
+  } else {
+    Loc = AccessSpecDecl->getEndLoc().getLocWithOffset(1);
+  }
+
+  DestructorString = (llvm::Twine(DestructorString) + "\nvirtual ~" +
+                      StructOrClass.getName().str() + "() = default;" +
+                      (AppendLineBreak ? "\n" : ""))
+                         .str();
+
+  return FixItHint::CreateInsertion(Loc, DestructorString);
+}
+
+static std::string getSourceText(const CXXDestructorDecl &Destructor) {
+  std::string SourceText;
+  llvm::raw_string_ostream DestructorStream(SourceText);
+  Destructor.print(DestructorStream);
+  return SourceText;
+}
+
+static std::string eraseKeyword(std::string &DestructorString,
+                                const std::string &Keyword) {
+  size_t KeywordIndex = DestructorString.find(Keyword);
+  if (KeywordIndex != std::string::npos)
+    DestructorString.erase(KeywordIndex, Keyword.length());
+  return DestructorString;
+}
+
+static FixItHint changePrivateDestructorVisibilityTo(
+    const std::string &Visibility, const CXXDestructorDecl &Destructor,
+    const SourceManager &SM, const LangOptions &LangOpts) {
+  std::string DestructorString =
+      (llvm::Twine() + Visibility + ":\n" +
+       (Visibility == "public" && !Destructor.isVirtual() ? "virtual " : ""))
+          .str();
+
+  std::string OriginalDestructor = getSourceText(Destructor);
+  if (Visibility == "protected" && Destructor.isVirtualAsWritten())
+    OriginalDestructor = eraseKeyword(OriginalDestructor, "virtual ");
+
+  DestructorString =
+      (llvm::Twine(DestructorString) + OriginalDestructor +
+       (Destructor.isExplicitlyDefaulted() ? ";\n" : "") + "private:")
+          .str();
+
+  /// Semicolons ending an explicitly defaulted destructor have to be deleted.
+  /// Otherwise, the left-over semicolon trails the \c private: access
+  /// specifier.
+  SourceLocation EndLocation;
+  if (Destructor.isExplicitlyDefaulted())
+    EndLocation =
+        utils::lexer::findNextTerminator(Destructor.getEndLoc(), SM, LangOpts)
+            .getLocWithOffset(1);
+  else
+    EndLocation = Destructor.getEndLoc().getLocWithOffset(1);
+
+  auto OriginalDestructorRange =
+      CharSourceRange::getCharRange(Destructor.getBeginLoc(), EndLocation);
+  return FixItHint::CreateReplacement(OriginalDestructorRange,
+                                      DestructorString);
+}
+
+void VirtualClassDestructorCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedClassOrStruct =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("ProblematicClassOrStruct");
+
+  const CXXDestructorDecl *Destructor = MatchedClassOrStruct->getDestructor();
+  if (!Destructor)
+    return;
+
+  if (Destructor->getAccess() == AccessSpecifier::AS_private) {
+    diag(MatchedClassOrStruct->getLocation(),
+         "destructor of %0 is private and prevents using the type")
+        << MatchedClassOrStruct;
+    diag(MatchedClassOrStruct->getLocation(),
+         /*FixDescription=*/"make it public and virtual", DiagnosticIDs::Note)
+        << changePrivateDestructorVisibilityTo(
+               "public", *Destructor, *Result.SourceManager, getLangOpts());
+    diag(MatchedClassOrStruct->getLocation(),
+         /*FixDescription=*/"make it protected", DiagnosticIDs::Note)
+        << changePrivateDestructorVisibilityTo(
+               "protected", *Destructor, *Result.SourceManager, getLangOpts());
+
+    return;
+  }
+
+  // Implicit destructors are public and non-virtual for classes and structs.
+  bool ProtectedAndVirtual = false;
+  FixItHint Fix;
+
+  if (MatchedClassOrStruct->hasUserDeclaredDestructor()) {
+    if (Destructor->getAccess() == AccessSpecifier::AS_public) {
+      Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
+    } else if (Destructor->getAccess() == AccessSpecifier::AS_protected) {
+      ProtectedAndVirtual = true;
+      Fix = FixItHint::CreateRemoval(getVirtualKeywordRange(
+          *Destructor, *Result.SourceManager, Result.Context->getLangOpts()));
+    }
+  } else {
+    Fix = generateUserDeclaredDestructor(*MatchedClassOrStruct,
+                                         *Result.SourceManager);
+  }
+
+  diag(MatchedClassOrStruct->getLocation(),
+       "destructor of %0 is %select{public and non-virtual|protected and "
+       "virtual}1")
+      << MatchedClassOrStruct << ProtectedAndVirtual;
+  diag(MatchedClassOrStruct->getLocation(),
+       "make it %select{public and virtual|protected and non-virtual}0",
+       DiagnosticIDs::Note)
+      << ProtectedAndVirtual << Fix;
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` check.
 
-  Finds potentially incorrect calls to ``memcmp()`` based on properties of the arguments.
+  Finds potentially incorrect calls to ``memcmp()`` based on properties of the
+  arguments.
+
+- New :doc:`cppcoreguidelines-virtual-class-destructor
+  <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` check.
+
+  Finds virtual classes whose destructor is neither public and virtual nor
+  protected and non-virtual.
 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
 
-  Reports identifiers whose names are too short. Currently checks local variables and function parameters only.
+  Reports identifiers whose names are too short. Currently checks local
+  variables and function parameters only.
+
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-exp42-c
   <clang-tidy/checks/cert-exp42-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-class-destructor.rst

+.. title:: clang-tidy - cppcoreguidelines-virtual-class-destructor
+
+cppcoreguidelines-virtual-class-destructor
+===============================================
+
+Finds virtual classes whose destructor is neither public and virtual
+nor protected and non-virtual. A virtual class's destructor should be specified
+in one of these ways to prevent undefined behaviour.
+
+This check implements
+`C.35 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual>`_
+from the CppCoreGuidelines.
+
+Note that this check will diagnose a class with a virtual method regardless of
+whether the class is used as a base class or not.
+
+Fixes are available for user-declared and implicit destructors that are either
+public and non-virtual or protected and virtual. No fixes are offered for
+private destructors. There, the decision whether to make them private and
+virtual or protected and non-virtual depends on the use case and is thus left
+to the user.
+
+Example
+-------
+
+For example, the following classes/structs get flagged by the check since they
+violate guideline **C.35**:
+
+.. code-block:: c++
+
+  struct Foo {        // NOK, protected destructor should not be virtual
+    virtual void f();
+  protected:
+    virtual ~Foo(){}
+  };
+
+  class Bar {         // NOK, public destructor should be virtual
+    virtual void f();
+  public:
+    ~Bar(){}
+  };
+
+This would be rewritten to look like this:
+
+.. code-block:: c++
+
+  struct Foo {        // OK, destructor is not virtual anymore
+    virtual void f();
+  protected:
+    ~Foo(){}
+  };
+
+  class Bar {         // OK, destructor is now virtual
+    virtual void f();
+  public:
+    virtual ~Bar(){}
+  };

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `cppcoreguidelines-pro-type-cstyle-cast <cppcoreguidelines-pro-type-cstyle-cast.html>`_, "Yes"
    `cppcoreguidelines-pro-type-member-init <cppcoreguidelines-pro-type-member-init.html>`_, "Yes"
    `cppcoreguidelines-pro-type-reinterpret-cast <cppcoreguidelines-pro-type-reinterpret-cast.html>`_,
    `cppcoreguidelines-pro-type-static-cast-downcast <cppcoreguidelines-pro-type-static-cast-downcast.html>`_, "Yes"
    `cppcoreguidelines-pro-type-union-access <cppcoreguidelines-pro-type-union-access.html>`_,
    `cppcoreguidelines-pro-type-vararg <cppcoreguidelines-pro-type-vararg.html>`_,
    `cppcoreguidelines-slicing <cppcoreguidelines-slicing.html>`_,
    `cppcoreguidelines-special-member-functions <cppcoreguidelines-special-member-functions.html>`_,
+   `cppcoreguidelines-virtual-class-destructor <cppcoreguidelines-virtual-class-destructor.html>`_, "Yes"
    `darwin-avoid-spinlock <darwin-avoid-spinlock.html>`_,
    `darwin-dispatch-once-nonstatic <darwin-dispatch-once-nonstatic.html>`_, "Yes"
    `fuchsia-default-arguments-calls <fuchsia-default-arguments-calls.html>`_,
    `fuchsia-default-arguments-declarations <fuchsia-default-arguments-declarations.html>`_, "Yes"
    `fuchsia-multiple-inheritance <fuchsia-multiple-inheritance.html>`_,
    `fuchsia-overloaded-operator <fuchsia-overloaded-operator.html>`_,
    `fuchsia-statically-constructed-objects <fuchsia-statically-constructed-objects.html>`_,
    `fuchsia-trailing-return <fuchsia-trailing-return.html>`_,

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

+// 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
+// As we have 2 conflicting fixes in notes, no fix is applied.
+struct PrivateVirtualBaseStruct {
+  virtual void f();
+
+private:
+  virtual ~PrivateVirtualBaseStruct() {}
+};
+
+struct PublicVirtualBaseStruct { // OK
+  virtual void f();
+  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
+struct ProtectedVirtualBaseStruct {
+  virtual void f();
+
+protected:
+  virtual ~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
+struct ProtectedVirtualDefaultBaseStruct {
+  virtual void f();
+
+protected:
+  virtual ~ProtectedVirtualDefaultBaseStruct() = default;
+  // 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
+// As we have 2 conflicting fixes in notes, no fix is applied.
+struct PrivateNonVirtualBaseStruct {
+  virtual void f();
+
+private:
+  ~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
+struct PublicNonVirtualBaseStruct {
+  virtual void f();
+  ~PublicNonVirtualBaseStruct() {}
+  // CHECK-FIXES: virtual ~PublicNonVirtualBaseStruct() {}
+};
+
+struct PublicNonVirtualNonBaseStruct { // OK according to C.35, since this struct does not have any virtual methods.
+  void f();
+  ~PublicNonVirtualNonBaseStruct() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PublicImplicitNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual
+// CHECK-FIXES: struct PublicImplicitNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~PublicImplicitNonVirtualBaseStruct() = default;
+struct PublicImplicitNonVirtualBaseStruct {
+  virtual void f();
+};
+
+// CHECK-MESSAGES: :[[@LINE+5]]:8: warning: destructor of 'PublicASImplicitNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+4]]:8: note: make it public and virtual
+// CHECK-FIXES: struct PublicASImplicitNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~PublicASImplicitNonVirtualBaseStruct() = default;
+// CHECK-FIXES-NEXT: private:
+struct PublicASImplicitNonVirtualBaseStruct {
+private:
+  virtual void f();
+};
+
+struct ProtectedNonVirtualBaseStruct { // OK
+  virtual void f();
+
+protected:
+  ~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
+// As we have 2 conflicting fixes in notes, no fix is applied.
+class PrivateVirtualBaseClass {
+  virtual void f();
+  virtual ~PrivateVirtualBaseClass() {}
+};
+
+class PublicVirtualBaseClass { // OK
+  virtual void f();
+
+public:
+  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
+class ProtectedVirtualBaseClass {
+  virtual void f();
+
+protected:
+  virtual ~ProtectedVirtualBaseClass() {}
+  // CHECK-FIXES: ~ProtectedVirtualBaseClass() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+5]]:7: warning: destructor of 'PublicImplicitNonVirtualBaseClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+4]]:7: note: make it public and virtual
+// CHECK-FIXES: public:
+// CHECK-FIXES-NEXT: virtual ~PublicImplicitNonVirtualBaseClass() = default;
+// CHECK-FIXES-NEXT: };
+class PublicImplicitNonVirtualBaseClass {
+  virtual void f();
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'PublicASImplicitNonVirtualBaseClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it public and virtual
+// CHECK-FIXES: public:
+// CHECK-FIXES-NEXT: virtual ~PublicASImplicitNonVirtualBaseClass() = default;
+// CHECK-FIXES-NEXT: int foo = 42;
+// CHECK-FIXES-NEXT: };
+class PublicASImplicitNonVirtualBaseClass {
+  virtual void f();
+
+public:
+  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
+class PublicNonVirtualBaseClass {
+  virtual void f();
+
+public:
+  ~PublicNonVirtualBaseClass() {}
+  // CHECK-FIXES: virtual ~PublicNonVirtualBaseClass() {}
+};
+
+class PublicNonVirtualNonBaseClass { // OK accoring to C.35, since this class does not have any virtual methods.
+  void f();
+
+public:
+  ~PublicNonVirtualNonBaseClass() {}
+};
+
+class ProtectedNonVirtualClass { // OK
+public:
+  virtual void f();
+
+protected:
+  ~ProtectedNonVirtualClass() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+7]]:7: warning: destructor of 'OverridingDerivedClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+6]]:7: note: make it public and virtual
+// CHECK-FIXES: class OverridingDerivedClass : ProtectedNonVirtualClass {
+// CHECK-FIXES-NEXT: public:
+// CHECK-FIXES-NEXT: virtual ~OverridingDerivedClass() = default;
+// CHECK-FIXES-NEXT: void f() override;
+// CHECK-FIXES-NEXT: };
+class OverridingDerivedClass : ProtectedNonVirtualClass {
+public:
+  void f() override; // is implicitly virtual
+};
+
+// CHECK-MESSAGES: :[[@LINE+7]]:7: warning: destructor of 'NonOverridingDerivedClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+6]]:7: note: make it public and virtual
+// CHECK-FIXES: class NonOverridingDerivedClass : ProtectedNonVirtualClass {
+// CHECK-FIXES-NEXT: void m();
+// CHECK-FIXES-NEXT: public:
+// CHECK-FIXES-NEXT: virtual ~NonOverridingDerivedClass() = default;
+// CHECK-FIXES-NEXT: };
+class NonOverridingDerivedClass : ProtectedNonVirtualClass {
+  void m();
+};
+// inherits virtual method
+
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'OverridingDerivedStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:8: note: make it public and virtual
+// CHECK-FIXES: struct OverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~OverridingDerivedStruct() = default;
+// CHECK-FIXES-NEXT: void f() override;
+// CHECK-FIXES-NEXT: };
+struct OverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+  void f() override; // is implicitly virtual
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'NonOverridingDerivedStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:8: note: make it public and virtual
+// CHECK-FIXES: struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~NonOverridingDerivedStruct() = default;
+// CHECK-FIXES-NEXT: void m();
+// CHECK-FIXES-NEXT: };
+struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+  void m();
+};
+// inherits virtual method