diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -26,6 +26,7 @@ ProTypeVarargCheck.cpp SlicingCheck.cpp SpecialMemberFunctionsCheck.cpp + VirtualBaseClassDestructorCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/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("cppcoreguidelines-slicing"); CheckFactories.registerCheck( "cppcoreguidelines-c-copy-assignment-signature"); + CheckFactories.registerCheck( + "cppcoreguidelines-virtual-base-class-destructor"); } ClangTidyOptions getModuleOptions() override { diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.h new file mode 100644 --- /dev/null +++ b/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 + +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 diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp new file mode 100644 --- /dev/null +++ b/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 + +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("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 + 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/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 + ` check. + + Finds base classes whose destructor is neither public and virtual nor + protected and non-virtual. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst new file mode 100644 --- /dev/null +++ b/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 `_ 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. + + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -162,6 +162,7 @@ `cppcoreguidelines-pro-type-vararg `_, `cppcoreguidelines-slicing `_, `cppcoreguidelines-special-member-functions `_, + `cppcoreguidelines-virtual-base-class-destructor `_, "Yes" `darwin-avoid-spinlock `_, `darwin-dispatch-once-nonstatic `_, "Yes" `fuchsia-default-arguments-calls `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp new file mode 100644 --- /dev/null +++ b/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(){}; +};