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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
