From d36a0333102698a1398971d0717465322b1c5c2c Mon Sep 17 00:00:00 2001 From: Anton Bikineev Date: Fri, 25 Oct 2019 13:03:53 +0200 Subject: [PATCH] [clang-tidy] New checker performance-trivially-destructible-check Checks for types which can be made trivially-destructible by removing out-of-line defaulted destructor declarations. The check is motivated by the work on C++ garbage collector in Blink (rendering engine for Chrome), which strives to minimize destructors and improve runtime of sweeping phase. In the entire chromium codebase the check hits over 2000 times. Differential Revision: https://reviews.llvm.org/D69435 --- .../clang-tidy/performance/CMakeLists.txt | 1 + .../performance/PerformanceTidyModule.cpp | 3 + .../performance/TriviallyDestructibleCheck.cpp | 82 +++++++++++++++++++++ .../performance/TriviallyDestructibleCheck.h | 40 +++++++++++ clang-tools-extra/clang-tidy/utils/Matchers.h | 4 ++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp | 16 ++++- clang-tools-extra/clang-tidy/utils/TypeTraits.h | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../checks/performance-trivially-destructible.rst | 15 ++++ .../performance-trivially-destructible.cpp | 84 ++++++++++++++++++++++ 11 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index b6302a5..cde2e24 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyPerformanceModule MoveConstructorInitCheck.cpp NoexceptMoveConstructorCheck.cpp PerformanceTidyModule.cpp + TriviallyDestructibleCheck.cpp TypePromotionInMathFnCheck.cpp UnnecessaryCopyInitialization.cpp UnnecessaryValueParamCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index f4b620a..269d09b 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -18,6 +18,7 @@ #include "MoveConstArgCheck.h" #include "MoveConstructorInitCheck.h" #include "NoexceptMoveConstructorCheck.h" +#include "TriviallyDestructibleCheck.h" #include "TypePromotionInMathFnCheck.h" #include "UnnecessaryCopyInitialization.h" #include "UnnecessaryValueParamCheck.h" @@ -47,6 +48,8 @@ public: "performance-move-constructor-init"); CheckFactories.registerCheck( "performance-noexcept-move-constructor"); + CheckFactories.registerCheck( + "performance-trivially-destructible"); CheckFactories.registerCheck( "performance-type-promotion-in-math-fn"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp new file mode 100644 index 0000000..5ed705b --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp @@ -0,0 +1,82 @@ +//===--- TriviallyDestructibleCheck.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 "TriviallyDestructibleCheck.h" +#include "../utils/LexerUtils.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; +using namespace clang::ast_matchers::internal; +using namespace clang::tidy::matchers; + +namespace clang { +namespace tidy { +namespace performance { + +namespace { + +AST_MATCHER(Decl, isFirstDecl) { return Node.isFirstDecl(); } + +AST_MATCHER_P(CXXRecordDecl, hasBase, Matcher, InnerMatcher) { + for (const CXXBaseSpecifier &BaseSpec : Node.bases()) { + QualType BaseType = BaseSpec.getType(); + if (InnerMatcher.matches(BaseType, Finder, Builder)) + return true; + } + return false; +} + +} // namespace + +void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + Finder->addMatcher( + cxxDestructorDecl( + isDefaulted(), + unless(anyOf(isFirstDecl(), isVirtual(), + ofClass(cxxRecordDecl( + anyOf(hasBase(unless(isTriviallyDestructible())), + has(fieldDecl(unless( + hasType(isTriviallyDestructible())))))))))) + .bind("decl"), + this); +} + +void TriviallyDestructibleCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("decl"); + + // Get locations of both first and out-of-line declarations. + SourceManager &SM = *Result.SourceManager; + const auto *FirstDecl = cast(MatchedDecl->getFirstDecl()); + const SourceLocation FirstDeclEnd = utils::lexer::findNextTerminator( + FirstDecl->getEndLoc(), SM, getLangOpts()); + const CharSourceRange SecondDeclRange = CharSourceRange::getTokenRange( + MatchedDecl->getBeginLoc(), + utils::lexer::findNextTerminator(MatchedDecl->getEndLoc(), SM, + getLangOpts())); + if (FirstDeclEnd.isInvalid() || SecondDeclRange.isInvalid()) + return; + + // Report diagnostic. + diag(FirstDecl->getLocation(), + "class %0 can be made trivially destructible by defaulting the " + "destructor on its first declaration") + << FirstDecl->getParent() + << FixItHint::CreateInsertion(FirstDeclEnd, " = default") + << FixItHint::CreateRemoval(SecondDeclRange); + diag(MatchedDecl->getLocation(), "destructor definition is here", + DiagnosticIDs::Note); +} + +} // namespace performance +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h new file mode 100644 index 0000000..ee3fb40 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h @@ -0,0 +1,40 @@ +//===--- TriviallyDestructibleCheck.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_PERFORMANCE_TRIVIALLYDESTRUCTIBLECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_TRIVIALLYDESTRUCTIBLECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace performance { + +/// A check that finds classes that would be trivial if not for the defaulted +/// destructors declared out-of-line: +/// struct A: TrivialClass { +/// ~A(); +/// TrivialClass trivial_fields; +/// }; +/// A::~A() = default; +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-trivially-destructible.html +class TriviallyDestructibleCheck : public ClangTidyCheck { +public: + TriviallyDestructibleCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_TRIVIALLYDESTRUCTIBLECHECK_H diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h index dbb72c9..e19c827 100644 --- a/clang-tools-extra/clang-tidy/utils/Matchers.h +++ b/clang-tools-extra/clang-tidy/utils/Matchers.h @@ -41,6 +41,10 @@ AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) { Node, Finder->getASTContext()); } +AST_MATCHER(QualType, isTriviallyDestructible) { + return utils::type_traits::isTriviallyDestructible(Node); +} + // Returns QualType matcher for references to const. AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) { using namespace ast_matchers; diff --git a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp index 9cc422a..0735587 100644 --- a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp +++ b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp @@ -54,7 +54,7 @@ bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, // Non-C++ records are always trivially constructible. if (!ClassDecl) return true; - // It is impossible to detemine whether an ill-formed decl is trivially + // It is impossible to determine whether an ill-formed decl is trivially // constructible. if (RecordDecl.isInvalidDecl()) return false; @@ -135,6 +135,20 @@ bool isTriviallyDefaultConstructible(QualType Type, const ASTContext &Context) { return false; } +// Based on QualType::isDestructedType. +bool isTriviallyDestructible(QualType Type) { + if (Type.isNull()) + return false; + + if (Type->isIncompleteType()) + return false; + + if (Type.getCanonicalType()->isDependentType()) + return false; + + return Type.isDestructedType() == QualType::DK_none; +} + bool hasNonTrivialMoveConstructor(QualType Type) { auto *Record = Type->getAsCXXRecordDecl(); return Record && Record->hasDefinition() && diff --git a/clang-tools-extra/clang-tidy/utils/TypeTraits.h b/clang-tools-extra/clang-tidy/utils/TypeTraits.h index 6102c28..f4d3455 100644 --- a/clang-tools-extra/clang-tidy/utils/TypeTraits.h +++ b/clang-tools-extra/clang-tidy/utils/TypeTraits.h @@ -28,6 +28,9 @@ bool isTriviallyDefaultConstructible(QualType Type, const ASTContext &Context); bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, const ASTContext &Context); +/// Returns `true` if `Type` is trivially destructible. +bool isTriviallyDestructible(QualType Type); + /// Returns true if `Type` has a non-trivial move constructor. bool hasNonTrivialMoveConstructor(QualType Type); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index c706ae1..4a7fef5 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -126,6 +126,12 @@ Improvements to clang-tidy Finds Objective-C implementations that implement ``-isEqual:`` without also appropriately implementing ``-hash``. +- New :doc:`performance-trivially-destructible + ` check. + + Finds types that could be made trivially-destructible by removing out-of-line + defaulted destructor declarations. + - Improved :doc:`bugprone-posix-return ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index bbdcdfa..2801168 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -342,6 +342,7 @@ Clang-Tidy Checks performance-move-const-arg performance-move-constructor-init performance-noexcept-move-constructor + performance-trivially-destructible performance-type-promotion-in-math-fn performance-unnecessary-copy-initialization performance-unnecessary-value-param diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst new file mode 100644 index 0000000..00e4e470 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst @@ -0,0 +1,15 @@ +.. title:: clang-tidy - performance-trivially-destructible + +performance-trivially-destructible +================================== + +Finds types that could be made trivially-destructible by removing out-of-line +defaulted destructor declarations. + +.. code-block:: c++ + + struct A: TrivialType { + ~A(); // Makes A non-trivially-destructible. + TrivialType trivial_fields; + }; + A::~A() = default; diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp new file mode 100644 index 0000000..927a090 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp @@ -0,0 +1,84 @@ +// RUN: %check_clang_tidy %s performance-trivially-destructible %t +// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp +// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -fix +// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -warnings-as-errors='-*,performance-trivially-destructible' + +struct TriviallyDestructible1 { + int a; +}; + +struct TriviallyDestructible2 : TriviallyDestructible1 { + ~TriviallyDestructible2() = default; + TriviallyDestructible1 b; +}; + +struct NotTriviallyDestructible1 : TriviallyDestructible2 { + ~NotTriviallyDestructible1(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'NotTriviallyDestructible1' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible] + // CHECK-FIXES: ~NotTriviallyDestructible1() = default; + TriviallyDestructible2 b; +}; + +NotTriviallyDestructible1::~NotTriviallyDestructible1() = default; // to-be-removed +// CHECK-MESSAGES: :[[@LINE-1]]:28: note: destructor definition is here +// CHECK-FIXES: {{^}}// to-be-removed + +// Don't emit for class template with type-dependent fields. +template +struct MaybeTriviallyDestructible1 { + ~MaybeTriviallyDestructible1() noexcept; + T t; +}; + +template +MaybeTriviallyDestructible1::~MaybeTriviallyDestructible1() noexcept = default; + +// Don't emit for specializations. +template struct MaybeTriviallyDestructible1; + +// Don't emit for class template with type-dependent bases. +template +struct MaybeTriviallyDestructible2 : T { + ~MaybeTriviallyDestructible2() noexcept; +}; + +template +MaybeTriviallyDestructible2::~MaybeTriviallyDestructible2() noexcept = default; + +// Emit for templates without dependent bases and fields. +template +struct MaybeTriviallyDestructible1 { + ~MaybeTriviallyDestructible1() noexcept; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'MaybeTriviallyDestructible1' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible] + // CHECK-FIXES: ~MaybeTriviallyDestructible1() noexcept = default; + TriviallyDestructible1 t; +}; + +template +MaybeTriviallyDestructible1::~MaybeTriviallyDestructible1() noexcept = default; // to-be-removed +// CHECK-MESSAGES: :[[@LINE-1]]:35: note: destructor definition is here +// CHECK-FIXES: {{^}}// to-be-removed + +// Emit for explicit specializations. +template <> +struct MaybeTriviallyDestructible1: TriviallyDestructible1 { + ~MaybeTriviallyDestructible1() noexcept; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'MaybeTriviallyDestructible1' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible] + // CHECK-FIXES: ~MaybeTriviallyDestructible1() noexcept = default; +}; + +MaybeTriviallyDestructible1::~MaybeTriviallyDestructible1() noexcept = default; // to-be-removed +// CHECK-MESSAGES: :[[@LINE-1]]:38: note: destructor definition is here +// CHECK-FIXES: {{^}}// to-be-removed + +struct NotTriviallyDestructible2 { + virtual ~NotTriviallyDestructible2(); +}; + +NotTriviallyDestructible2::~NotTriviallyDestructible2() = default; + +struct NotTriviallyDestructible3: NotTriviallyDestructible2 { + ~NotTriviallyDestructible3(); +}; + +NotTriviallyDestructible3::~NotTriviallyDestructible3() = default; -- 2.7.4