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

Mon Oct 4 11:53:28 PDT 2021

mgartmann added a comment.

Please excuse my late reply! I have been on vacation for the last two weeks and didn't have the time to respond to this thread until now.

In D110614#3032760 <https://reviews.llvm.org/D110614#3032760>, @carlosgalvezp wrote:

> So the derived destructor only shows up in `ClassTemplateSpecializationDecl` - the `CXXRecordDecl` doesn't have it (therefore implicit public non-virtual) and that's why the check triggers.

I came to the same conclusion with a quick troubleshoot. Honestly, I didn't expect SEMA to behave this way and to generate CXXRecordDecl nodes without any CXXDestructorDecl sub-node. 
As a rather novice C++ programmer, I didn't have the described scenarios in mind when I initially created the check. Hence, the needed tests to find these false-positives beforehand were missing. 
I would like to apologise to everyone for the trouble this has caused.

@carlosgalvezp Thanks a lot for putting your time and effort into fix this bug!

In D110614#3038519 <https://reviews.llvm.org/D110614#3038519>, @carlosgalvezp wrote:

> Anyway this check will need to be extended in the future, since the C++ Core Guidelines has added a new bullet in their "Enforcement" section:
> https://github.com/isocpp/CppCoreGuidelines/commit/e44a9fcbd40923e9d5d342e444cf8a811e4a3eae

Thanks for pointing this out to me. Let me know if I can help you with this in any way. I would be available to implement the new enforcement from 2021-10-11 on.

Again, my dearest apologies for this bug.

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

https://reviews.llvm.org/D110614