mgartmann created this revision.
mgartmann added reviewers: aaron.ballman, njames93, alexfh.
mgartmann added a project: clang-tools-extra.
Herald added subscribers: shchenz, kbarton, xazax.hun, mgorny, nemanjai.
mgartmann requested review of this revision.
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
<https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual>
from the CppCoreGuidelines.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D102325
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp
@@ -0,0 +1,131 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-virtual-base-class-destructor %t
+
+// CHECK-MESSAGES: :[[@LINE+1]]:8: warning: destructor of struct 'PrivateVirtualBaseStruct' is private and prevents using the type. Consider making it public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+struct PrivateVirtualBaseStruct {
+ virtual void f();
+
+private:
+ virtual ~PrivateVirtualBaseStruct(){};
+};
+
+struct PublicVirtualBaseStruct { // OK
+ virtual void f();
+ virtual ~PublicVirtualBaseStruct(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+1]]:8: warning: destructor of struct 'ProtectedVirtualBaseStruct' is protected and virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+struct ProtectedVirtualBaseStruct {
+ virtual void f();
+
+protected:
+ virtual ~ProtectedVirtualBaseStruct(){};
+ // CHECK-FIXES: ~ProtectedVirtualBaseStruct(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+1]]:8: warning: destructor of struct 'PrivateNonVirtualBaseStruct' is private and prevents using the type. Consider making it public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+struct PrivateNonVirtualBaseStruct {
+ virtual void f();
+
+private:
+ ~PrivateNonVirtualBaseStruct(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+1]]:8: warning: destructor of struct 'PublicNonVirtualBaseStruct' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+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+3]]:8: warning: destructor of struct 'PublicImplicitNonVirtualBaseStruct' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// CHECK-FIXES: struct PublicImplicitNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~PublicImplicitNonVirtualBaseStruct() = default;
+struct PublicImplicitNonVirtualBaseStruct {
+ virtual void f();
+};
+
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of struct 'PublicASImplicitNonVirtualBaseStruct' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// 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+1]]:7: warning: destructor of class 'PrivateVirtualBaseClass' is private and prevents using the type. Consider making it public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+class PrivateVirtualBaseClass {
+ virtual void f();
+ virtual ~PrivateVirtualBaseClass(){};
+};
+
+class PublicVirtualBaseClass { // OK
+ virtual void f();
+
+public:
+ virtual ~PublicVirtualBaseClass(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: destructor of class 'ProtectedVirtualBaseClass' is protected and virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+class ProtectedVirtualBaseClass {
+ virtual void f();
+
+protected:
+ virtual ~ProtectedVirtualBaseClass(){};
+ // CHECK-FIXES: ~ProtectedVirtualBaseClass(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+4]]:7: warning: destructor of class 'PublicImplicitNonVirtualBaseClass' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// CHECK-FIXES: public:
+// CHECK-FIXES-NEXT: virtual ~PublicImplicitNonVirtualBaseClass() = default;
+// CHECK-FIXES-NEXT: };
+class PublicImplicitNonVirtualBaseClass {
+ virtual void f();
+};
+
+// CHECK-MESSAGES: :[[@LINE+5]]:7: warning: destructor of class 'PublicASImplicitNonVirtualBaseClass' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// 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+1]]:7: warning: destructor of class 'PublicNonVirtualBaseClass' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+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
+ virtual void f();
+
+protected:
+ ~ProtectedNonVirtualClass(){};
+};
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -162,6 +162,7 @@
`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-base-class-destructor <cppcoreguidelines-virtual-base-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>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - cppcoreguidelines-virtual-base-class-destructor
+
+cppcoreguidelines-virtual-base-class-destructor
+===============================================
+
+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.
+
+This check implements `C.35 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual>`_ from the CppCoreGuidelines.
+
+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(){};
+ };
+
+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.
+
+Options
+-------
+
+.. option:: IndentationWidth
+
+ An unsigned integer specifying how wide an indentation is in terms of blank spaces.
+ This option is used for classes/structs with implicit destructors, where a
+ used-declared destructor needs to be inserted.
+
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -95,6 +95,12 @@
Finds member initializations in the constructor body which can be placed into
the initialization list instead.
+- New :doc:`cppcoreguidelines-virtual-base-class-destructor
+ <clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor>` check.
+
+ Finds base classes whose destructor is neither public and virtual nor
+ protected and non-virtual.
+
New check aliases
^^^^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.h
@@ -0,0 +1,56 @@
+//===--- VirtualBaseClassDestructorCheck.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_VIRTUALBASECLASSDESTRUCTORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_VIRTUALBASECLASSDESTRUCTORCHECK_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-base-class-destructor.html
+class VirtualBaseClassDestructorCheck : public ClangTidyCheck {
+ const unsigned IndentationWidth;
+ CharSourceRange getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) const;
+ FixItHint
+ generateUserDeclaredDestructor(const CXXRecordDecl &StructOrClass,
+ const SourceManager &SourceManager) const;
+ AccessSpecDecl *getPublicASDecl(const CXXRecordDecl &StructOrClass) const;
+ std::string indent(int Indentation) const;
+
+public:
+ VirtualBaseClassDestructorCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IndentationWidth(Options.get("IndentationWidth", 4)) {}
+
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
+ Options.store(Opts, "IndentationWidth", IndentationWidth);
+ }
+
+ 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_VIRTUALBASECLASSDESTRUCTORCHECK_H
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
@@ -0,0 +1,149 @@
+//===--- VirtualBaseClassDestructorCheck.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 "VirtualBaseClassDestructorCheck.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 VirtualBaseClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ cxxRecordDecl(
+ anyOf(isStruct(), isClass()), has(cxxMethodDecl(isVirtual())),
+ unless(anyOf(
+ has(cxxDestructorDecl(isPublic(), isVirtual())),
+ has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
+ .bind("ProblematicClassOrStruct"),
+ this);
+}
+
+void VirtualBaseClassDestructorCheck::check(
+ const MatchFinder::MatchResult &Result) {
+
+ const auto *MatchedClassOrStruct =
+ Result.Nodes.getNodeAs<CXXRecordDecl>("ProblematicClassOrStruct");
+
+ const auto Destructor = MatchedClassOrStruct->getDestructor();
+
+ if (Destructor->getAccess() == AS_private) {
+ diag(MatchedClassOrStruct->getLocation(),
+ "destructor of %0 %1 is private and prevents using the type. Consider "
+ "making it public and virtual or protected and non-virtual")
+ << (MatchedClassOrStruct->isClass() ? "class" : "struct")
+ << MatchedClassOrStruct;
+
+ return;
+ }
+
+ // Implicit destructors are public and non-virtual for classes and structs.
+ std::string TypeAndVirtuality = "public and non-virtual";
+ FixItHint Fix;
+
+ if (MatchedClassOrStruct->hasUserDeclaredDestructor()) {
+ if (Destructor->getAccess() == AccessSpecifier::AS_public) {
+ Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
+ } else if (Destructor->getAccess() == AS_protected) {
+ TypeAndVirtuality = "protected and virtual";
+ Fix = FixItHint::CreateRemoval(getVirtualKeywordRange(
+ *Destructor, *Result.SourceManager, Result.Context->getLangOpts()));
+ }
+ } else {
+ Fix = generateUserDeclaredDestructor(*MatchedClassOrStruct,
+ *Result.SourceManager);
+ }
+
+ diag(MatchedClassOrStruct->getLocation(),
+ "destructor of %0 %1 is %2. It should either be public and virtual or "
+ "protected and non-virtual")
+ << (MatchedClassOrStruct->isClass() ? "class" : "struct")
+ << MatchedClassOrStruct << TypeAndVirtuality << Fix;
+}
+
+CharSourceRange VirtualBaseClassDestructorCheck::getVirtualKeywordRange(
+ const CXXDestructorDecl &Destructor, const SourceManager &SM,
+ const LangOptions &LangOpts) const {
+ auto VirtualBeginLoc = Destructor.getBeginLoc();
+ auto VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
+ Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
+
+ /// Range ends with \c StartOfNextToken so that any whitespace after \c
+ /// virtual is included.
+ auto StartOfNextToken = Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
+ .getValue()
+ .getLocation();
+
+ CharSourceRange Range = CharSourceRange{};
+ Range.setBegin(VirtualBeginLoc);
+ Range.setEnd(StartOfNextToken);
+ return Range;
+}
+
+FixItHint VirtualBaseClassDestructorCheck::generateUserDeclaredDestructor(
+ const CXXRecordDecl &StructOrClass,
+ const SourceManager &SourceManager) const {
+ auto DestructorString = std::string("");
+ SourceLocation Loc;
+ bool AppendLineBreak = false;
+ const unsigned ColumnOffset = 1;
+
+ auto ParentIndentation =
+ SourceManager.getExpansionColumnNumber(StructOrClass.getBeginLoc()) -
+ ColumnOffset;
+
+ auto AccessSpecDecl = getPublicASDecl(StructOrClass);
+
+ if (!AccessSpecDecl) {
+ if (StructOrClass.isClass()) {
+ Loc = StructOrClass.getEndLoc();
+ DestructorString.append("public:");
+ AppendLineBreak = true;
+ } else {
+ Loc = StructOrClass.getBraceRange().getBegin().getLocWithOffset(1);
+ }
+ } else {
+ Loc = AccessSpecDecl->getEndLoc().getLocWithOffset(1);
+ }
+
+ DestructorString.append("\n")
+ .append(indent(ParentIndentation + IndentationWidth))
+ .append("virtual ~")
+ .append(StructOrClass.getName().str())
+ .append("() = default;")
+ .append(AppendLineBreak ? "\n" + indent(ParentIndentation) : "");
+
+ return FixItHint::CreateInsertion(Loc, DestructorString);
+}
+
+AccessSpecDecl *VirtualBaseClassDestructorCheck::getPublicASDecl(
+ const CXXRecordDecl &StructOrClass) const {
+ 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;
+}
+
+std::string VirtualBaseClassDestructorCheck::indent(int Indentation) const {
+ return std::string().append(Indentation, ' ');
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -35,6 +35,7 @@
#include "ProTypeVarargCheck.h"
#include "SlicingCheck.h"
#include "SpecialMemberFunctionsCheck.h"
+#include "VirtualBaseClassDestructorCheck.h"
namespace clang {
namespace tidy {
@@ -94,6 +95,8 @@
CheckFactories.registerCheck<SlicingCheck>("cppcoreguidelines-slicing");
CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
"cppcoreguidelines-c-copy-assignment-signature");
+ CheckFactories.registerCheck<VirtualBaseClassDestructorCheck>(
+ "cppcoreguidelines-virtual-base-class-destructor");
}
ClangTidyOptions getModuleOptions() override {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -26,6 +26,7 @@
ProTypeVarargCheck.cpp
SlicingCheck.cpp
SpecialMemberFunctionsCheck.cpp
+ VirtualBaseClassDestructorCheck.cpp
LINK_LIBS
clangTidy
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
