mgartmann updated this revision to Diff 345635.
mgartmann added a comment.
Resolved readability-identifier-naming warning, adjusted check's documentation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102325/new/
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,173 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-virtual-base-class-destructor %t
+
+// CHECK-MESSAGES: :[[@LINE+1]]:8: warning: destructor of '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 '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 '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 '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 '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 '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 '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 '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 '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 '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 '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
+public:
+ virtual void f();
+
+protected:
+ ~ProtectedNonVirtualClass(){};
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'OverridingDerivedClass' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// 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+6]]:7: warning: destructor of 'NonOverridingDerivedClass' is public and non-virtual. It should either be public and virtual or protected and non-virtual [cppcoreguidelines-virtual-base-class-destructor]
+// 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+5]]:8: warning: destructor of 'OverridingDerivedStruct' 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 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+5]]:8: warning: destructor of 'NonOverridingDerivedStruct' 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 NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~NonOverridingDerivedStruct() = default;
+// CHECK-FIXES-NEXT: void m();
+// CHECK-FIXES-NEXT: };
+struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
+ void m();
+};
+// inherits virtual method
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
@@ -165,6 +165,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,52 @@
+.. 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.
+
+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(){};
+ };
+
+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.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -119,6 +119,12 @@
Finds calls to ``new`` with missing exception handler for ``std::bad_alloc``.
+- 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,41 @@
+//===--- 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 {
+public:
+ VirtualBaseClassDestructorCheck(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_VIRTUALBASECLASSDESTRUCTORCHECK_H
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
@@ -0,0 +1,143 @@
+//===--- 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) {
+ ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
+ hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
+
+ Finder->addMatcher(
+ cxxRecordDecl(
+ anyOf(isStruct(), isClass()),
+ 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();
+
+ CharSourceRange Range = CharSourceRange{};
+ Range.setBegin(VirtualBeginLoc);
+ Range.setEnd(StartOfNextToken);
+ return Range;
+}
+
+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.append("public:");
+ AppendLineBreak = true;
+ } else {
+ Loc = StructOrClass.getBraceRange().getBegin().getLocWithOffset(1);
+ }
+ } else {
+ Loc = AccessSpecDecl->getEndLoc().getLocWithOffset(1);
+ }
+
+ DestructorString.append("\n")
+ .append("virtual ~")
+ .append(StructOrClass.getName().str())
+ .append("() = default;")
+ .append(AppendLineBreak ? "\n" : "");
+
+ return FixItHint::CreateInsertion(Loc, DestructorString);
+}
+
+void VirtualBaseClassDestructorCheck::check(
+ const MatchFinder::MatchResult &Result) {
+
+ const auto *MatchedClassOrStruct =
+ Result.Nodes.getNodeAs<CXXRecordDecl>("ProblematicClassOrStruct");
+
+ const CXXDestructorDecl *Destructor = MatchedClassOrStruct->getDestructor();
+
+ if (Destructor->getAccess() == AS_private) {
+ diag(MatchedClassOrStruct->getLocation(),
+ "destructor of %0 is private and prevents using the type. Consider "
+ "making it public and virtual or protected and non-virtual")
+ << MatchedClassOrStruct;
+
+ return;
+ }
+
+ // Implicit destructors are public and non-virtual for classes and structs.
+ bool ProtectedVirtual = false;
+ FixItHint Fix;
+
+ if (MatchedClassOrStruct->hasUserDeclaredDestructor()) {
+ if (Destructor->getAccess() == AccessSpecifier::AS_public) {
+ Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
+ } else if (Destructor->getAccess() == AS_protected) {
+ ProtectedVirtual = 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. It should either be public and virtual or protected and "
+ "non-virtual")
+ << MatchedClassOrStruct << ProtectedVirtual << Fix;
+}
+
+} // 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
