[clang-tidy] New checker performance-trivially-destructible-check

[AntonBikineev]

Summary

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 in Chrome), which strives to minimize destructor usage
and improve runtime of sweeping phase.

In the entire chromium codebase the check hits over ~2500 times.

[lebedev.ri]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

23

I believe static functions are preferred to anon namespaces;
most (if not all) of this checking should/could be done in the matcher itself,
i.e. the check() could be called to unconditionally issue diag.

51

https://en.cppreference.com/w/cpp/language/destructor says defaulted destructor is C++11 feature,
so not register if this isn't C++11?

78

This always drops out-of-line defaulting.
Is there/can there be any case where such defaultig should just be moved in-line into the class?

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h

19–20

This doesn't read right, should be closer to this maybe

/// A check that finds classes that would be trivial if not for the defaulted destructors declared out-of-line:

clang-tools-extra/docs/ReleaseNotes.rst

142

s/destrictuble/destructible/?

clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst

7

s/destrictuble/destructible/?

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp

2

Do you also want to check that fix-it applies and result passes sema?

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

30

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

32

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

39

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

62

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

67

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

clang-tools-extra/docs/ReleaseNotes.rst

139

Please move into new checks list (in alphabetical order).

[AntonBikineev]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

23

I couldn't find matchers to check triviallity of special functions or something like isFirstDecl(). Perhaps, would make sense to add some.

51

Right. However, I think it will also make sense to extend the check for destructors with empty bodies.

78

I've thought about this. I think it mostly comes down to code-style preference (rule-of-5 vs rule-of-0). But after giving it another thought, I think, the current implementation may lead to more surprises for the user, so probably moving = default to the first declaration would be a better idea :)

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp

2

Aren't fix-its already checked by CHECK-FIXES?
It would be great to check if the result is compilable. Should I run another clang_tidy instance to check that or is there another way to do so?

[AntonBikineev]

Thanks for the suggestions! Addressed some of them.

[AntonBikineev]

Roman, could you please take another look at it?

[lebedev.ri]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

23

You can trivially add your own matchers, see e.g.
https://github.com/llvm/llvm-project/blob/058b628264d276515a0aab66285816fe6cde2aa2/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp#L15-L39

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp

2

It FileChecks the // CHECK-FIXES lines, but i don't think it actually applies them.
https://github.com/llvm/llvm-project/blob/885c559369fe3d6323898c17787bd0454065fc34/clang-tools-extra/test/clang-tidy/checkers/cert-uppercase-literal-suffix-integer.cpp#L2-L4

[aaron.ballman]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

27

Drop top-level const qualifiers unless it's a pointer or reference (it's not a style we use elsewhere).

39

We don't usually attach names to TODOs -- are you planning to work on that for this patch?

71

it -> its

[AntonBikineev]

Thanks for the suggestions!

Added isTriviallyDestructible matcher to utils/Matchers.h.

[AntonBikineev]

Does anybody have suggestions on this?

[lebedev.ri]

In D69435#1725795, @AntonBikineev wrote:

Does anybody have suggestions on this?

Can you be more specific what the question is?

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

43

Thanks, this looks good.

[AntonBikineev]

Roman, thanks for reviewing and the ideas.

Can you be more specific what the question is?

I meant to ping if anybody has more comments on this patch. Otherwise I would go ahead and submit it.

[sylvestre.ledru]

Did you see some significant perf improvements for Chromium?

[AntonBikineev]

Did you see some significant perf improvements for Chromium?

I don't see major improvements in browser benchmarks like Speedometer, but in the Blink garbage collector, which queues destructors to be executed later, this change proved to reduce the number of them by 5%. That, in turn, reduces GC time on the main thread which is significant for latency.

[sylvestre.ledru]

Thanks!

[AntonBikineev]

BTW, this very contrived benchmark shows 5x: http://quick-bench.com/sczBi_lVndKut9jOj4UofC0HYew

[xbolva00]

In D69435#1728674, @AntonBikineev wrote:

BTW, this very contrived benchmark shows 5x: http://quick-bench.com/sczBi_lVndKut9jOj4UofC0HYew

There is no difference in perf for GCC.

[AntonBikineev]

There is no difference in perf for GCC.

Yes, but that's because gcc still optimizes the call to noinlined function. In the common scenario which this check addresses (destructor defaulted in .cpp file) gcc is not able to optimize the call.

[gribozavr2]

LGTM modulo the isFirstDeclComment. Will approve after we resolve that discussion.

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

46

The "isFirstDecl" part probably will always work, however, it is semantically not really the right predicate to check. What we want to check is where the constructor is declared -- in the class or outside the class, correct? If so, then the matcher should be looking at the DeclContext of the constructor decl.

[AntonBikineev]

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

46

Thanks for the comment.

What we want to check is where the constructor is declared -- in the class or outside the class, correct?

I was following the Standard's wording, which says that trivial destructor is not user-provided, where user-provided means

user-declared and not explicitly defaulted or deleted on its first declaration.

Technically, I think both ways should work as C++ doesn't allow more than 2 special function declarations.

[gribozavr2]

Alright then. (Although I don't know whether the redeclaration chain always models that wording in the standard, and what that wording means when we don't have a linear ordering of text -- that is, in modules.)

Do you have commit access or do you need me to commit for you?

[AntonBikineev]

In D69435#1729580, @gribozavr2 wrote:

Alright then. (Although I don't know whether the redeclaration chain always models that wording in the standard, and what that wording means when we don't have a linear ordering of text -- that is, in modules.)

Do you have commit access or do you need me to commit for you?

Thanks for taking the look, I'll commit.

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitConversionInLoopCheck.cpp
   InefficientAlgorithmCheck.cpp
   InefficientStringConcatenationCheck.cpp
   InefficientVectorOperationCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   PerformanceTidyModule.cpp
+  TriviallyDestructibleCheck.cpp
   TypePromotionInMathFnCheck.cpp
   UnnecessaryCopyInitialization.cpp
   UnnecessaryValueParamCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

 #include "ForRangeCopyCheck.h"
 #include "ImplicitConversionInLoopCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "InefficientStringConcatenationCheck.h"
 #include "InefficientVectorOperationCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InefficientVectorOperationCheck>(
         "performance-inefficient-vector-operation");
     CheckFactories.registerCheck<MoveConstArgCheck>(
         "performance-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "performance-move-constructor-init");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "performance-noexcept-move-constructor");
+    CheckFactories.registerCheck<TriviallyDestructibleCheck>(
+        "performance-trivially-destructible");
     CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
         "performance-type-promotion-in-math-fn");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");
     CheckFactories.registerCheck<UnnecessaryValueParamCheck>(
         "performance-unnecessary-value-param");
   }
 };

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp

+//===--- 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 "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+static bool
+CheckPotentiallyTriviallyDestructible(const CXXDestructorDecl *Dtor) {
+  if (Dtor->isFirstDecl())
+    return false;
+  // Check direct base classes.
+  const CXXRecordDecl *RecordDecl = Dtor->getParent();
+  for (FieldDecl *Field : RecordDecl->fields()) {
+    const QualType FieldType = Field->getType();
+    if (FieldType->isDependentType() ||
+        FieldType.isDestructedType() != clang::QualType::DK_none)
+      return false;
+  }
+  // Check fields.
+  for (const CXXBaseSpecifier &BaseSpec : RecordDecl->bases()) {
+    const QualType BaseType = BaseSpec.getType();
+    if (BaseType->isDependentType() ||
+        BaseType.isDestructedType() != clang::QualType::DK_none)
+      return false;
+  }
+  // TODO(bikineev): Check for empty compound statement?
+  return true;
+}
+
+void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  Finder->addMatcher(
+      cxxDestructorDecl(isDefaulted(), unless(isVirtual())).bind("decl"), this);
+}
+
+void TriviallyDestructibleCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXDestructorDecl>("decl");
+  if (!CheckPotentiallyTriviallyDestructible(MatchedDecl))
+    return;
+
+  // Get locations of both first and out-of-line declarations.
+  SourceManager &SM = *Result.SourceManager;
+  const auto *FirstDecl = cast<CXXMethodDecl>(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 it 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

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-register-over-unsigned
   them to use ``Register``
 
 - New :doc:`objc-missing-hash
   <clang-tidy/checks/objc-missing-hash>` check.
 
   Finds Objective-C implementations that implement ``-isEqual:`` without also
   appropriately implementing ``-hash``.
 
+- New :doc:`performance-trivially-destructible
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
 - The 'objc-avoid-spinlock' check was renamed to :doc:`darwin-avoid-spinlock
   <clang-tidy/checks/darwin-avoid-spinlock>`
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------

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

.. toctree::
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    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
    portability-simd-intrinsics
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-const-return-type
    readability-container-size-empty

clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst

+.. 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;

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp

+// RUN: %check_clang_tidy %s performance-trivially-destructible %t
+
+struct TriviallyDestructible1 {
+  int a;
+};
+
+struct TriviallyDestructible2 : TriviallyDestructible1 {
+  TriviallyDestructible1 b;
+};
+
+struct NotTriviallyDestructible1 : TriviallyDestructible2 {
+  ~NotTriviallyDestructible1();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'NotTriviallyDestructible1' can be made trivially destructible by defaulting the destructor on it 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 <class T>
+struct MaybeTriviallyDestructible1 {
+  ~MaybeTriviallyDestructible1() noexcept;
+  T t;
+};
+
+template <class T>
+MaybeTriviallyDestructible1<T>::~MaybeTriviallyDestructible1() noexcept = default;
+
+// Don't emit for specializations.
+template struct MaybeTriviallyDestructible1<int>;
+
+// Don't emit for class template with type-dependent bases.
+template <class T>
+struct MaybeTriviallyDestructible2 : T {
+  ~MaybeTriviallyDestructible2() noexcept;
+};
+
+template <class T>
+MaybeTriviallyDestructible2<T>::~MaybeTriviallyDestructible2() noexcept = default;
+
+// Emit for templates without dependent bases and fields.
+template <class T>
+struct MaybeTriviallyDestructible1<T *> {
+  ~MaybeTriviallyDestructible1() noexcept;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'MaybeTriviallyDestructible1<T *>' can be made trivially destructible by defaulting the destructor on it first declaration [performance-trivially-destructible]
+  // CHECK-FIXES: ~MaybeTriviallyDestructible1() noexcept = default;
+  TriviallyDestructible1 t;
+};
+
+template <class T>
+MaybeTriviallyDestructible1<T *>::~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<double>: TriviallyDestructible1 {
+  ~MaybeTriviallyDestructible1() noexcept;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'MaybeTriviallyDestructible1<double>' can be made trivially destructible by defaulting the destructor on it first declaration [performance-trivially-destructible]
+  // CHECK-FIXES: ~MaybeTriviallyDestructible1() noexcept = default;
+};
+
+MaybeTriviallyDestructible1<double>::~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;