[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

Wed Sep 29 00:24:10 PDT 2021

carlosgalvezp added a subscriber: whisperity.
carlosgalvezp added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26

+  ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualDestructor =
+      hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual())))));
----------------
aaron.ballman wrote:
> I'm confused as to why this is necessary -- this AST matcher calls through to `CXXMethodDecl::isVirtual()` which is different from `isVirtualAsWritten()` and should already account for inheriting the virtual specifier.
In the Bug report, @whisperity mentioned this problem could be somewhere else:

> Nevertheless, if you check the AST for the test code at http://godbolt.org/z/6MqG4Kb85, we can see that the derived destructor is in fact **not** marked virtual. So it's Sema which does not give this flag to the AST-clients properly.

I don't even know what Sema is so I don't really know how to fix it at that level, and I wonder if it would break other things.

If anyone has some time to walk me through where the fix should go I'm happy to try it out!

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

https://reviews.llvm.org/D110614