[PATCH] D69435: [clang-tidy] New checker performance-trivially-destructible-check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:22
+
+bool CheckPotentiallyTriviallyDestructible(const CXXDestructorDecl *Dtor) {
+  if (Dtor->isFirstDecl() || !Dtor->isExplicitlyDefaulted())
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:50
+void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxDestructorDecl().bind("decl"), this);
+}
----------------
https://en.cppreference.com/w/cpp/language/destructor says defaulted destructor is C++11 feature,
so not register if this isn't C++11?


================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:77
+      << FirstDecl << FixItHint::CreateRemoval(FirstDeclRange)
+      << FixItHint::CreateRemoval(SecondDeclRange);
+  diag(MatchedDecl->getLocation(), "destructor definition is here",
----------------
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?



================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h:18-19
+
+/// A check that finds out-of-line declared defaulted destructors, which would
+/// otherwise be trivial:
+/// struct A: TrivialClass {
----------------
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:
```


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
+
+  Finds types that could be made trivially-destrictuble by removing out-of-line
+  defaulted destructor declarations.
----------------
s/destrictuble/destructible/?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst:6
+
+Finds types that could be made trivially-destrictuble by removing out-of-line
+defaulted destructor declarations.
----------------
s/destrictuble/destructible/?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp:1
+// RUN: %check_clang_tidy %s performance-trivially-destructible %t
+
----------------
Do you also want to check that fix-it applies and result passes sema?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69435/new/

https://reviews.llvm.org/D69435