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

[Anton Bikineev via Phabricator via cfe-commits]

AntonBikineev marked 9 inline comments as done.
AntonBikineev added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:22
+
+bool CheckPotentiallyTriviallyDestructible(const CXXDestructorDecl *Dtor) {
+  if (Dtor->isFirstDecl() || !Dtor->isExplicitlyDefaulted())
----------------
lebedev.ri wrote:
> 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.
I couldn't find matchers to check triviallity of special functions or something like isFirstDecl(). Perhaps, would make sense to add some.


================
Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:50
+void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxDestructorDecl().bind("decl"), this);
+}
----------------
lebedev.ri wrote:
> https://en.cppreference.com/w/cpp/language/destructor says defaulted destructor is C++11 feature,
> so not register if this isn't C++11?
Right. However, I think it will also make sense to extend the check for destructors with empty bodies.


================
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",
----------------
lebedev.ri wrote:
> 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?
> 
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 :)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp:1
+// RUN: %check_clang_tidy %s performance-trivially-destructible %t
+
----------------
lebedev.ri wrote:
> Do you also want to check that fix-it applies and result passes sema?
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?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69435