[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

lebedev.ri added a comment. Question: should this really diagnose classes that are marked as `final`? Surely such a class can never be a base class? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102325/new/ https://reviews.llvm.org/D102325

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

This revision was automatically updated to reflect the committed changes. Closed by commit rGc58c7a6ea053: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check (authored by mgartmann, committed by whisperity). Changed prior to commit:

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

whisperity added a comment. In D102325#2981760 , @mgartmann wrote: > @whisperity Thanks a lot for helping me out! Sorry I got busy with a few official business and then got deeply distracted by what I'm working on, but I'll get to committing this ASAP.

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann added a comment. @whisperity Thanks a lot for helping me out! Name: Marco Gartmann E-Mail: gartmannma...@hotmail.com Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102325/new/ https://reviews.llvm.org/D102325

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

whisperity added a comment. In D102325#2970683 , @mgartmann wrote: > @aaron.ballman Thanks a lot for your review! > > Can somebody help me merging this change? I do not have the rights to do so. > Thanks in advanve! Sure, just please specify what name

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann added a comment. @aaron.ballman Thanks a lot for your review! Can somebody help me merging this change? I do not have the rights to do so. Thanks in advanve! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102325/new/

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D102325#2816919 , @mgartmann wrote: > I assume that these destructors are private by on purpose. Please correct me > if I am wrong.

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann updated this revision to Diff 365161. mgartmann added a comment. - Updated this patch with the current state of the LLVM project GitHub repository Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102325/new/

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann added a comment. Thanks a lot for your feedback, @aaron.ballman! I was able to incorporate it in the updated diff. On your advice, I also ran this check on a large open source project using the `run-clang-tidy.py` script. I decided to do it on the llvm project itself as follows:

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann updated this revision to Diff 351869. mgartmann marked 10 inline comments as done. mgartmann added a comment. - changed name of check to `cppcoreguidelines-virtual-class-destructor` - removed matcher for class or struct in AST matcher - changed string concatenations to use `llvm::twine`

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

aaron.ballman added a comment. Have you tried running this over any large C++ code bases to see how often the diagnostics trigger and whether there are false positives? If not, you should probably do so -- one concern I have is with a private virtual destructor; I could imagine people using

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann updated this revision to Diff 348988. mgartmann added a comment. - added fixes for private destructors - separated fixes for private destructors into notes - added a test case for a `= default;` constructor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

aaron.ballman added a comment. FWIW, this looks like it's also partially covering `-Wnon-virtual-dtor`, but that doesn't seem to have the checks for a protected destructor. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:110-111

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann marked an inline comment as done. mgartmann added a comment. Dear reviewers, Since this work was conducted as part of a Bachelor's thesis, which has to be handed in on the 18th of June, 2021, we wanted to add a friendly ping to this patch. It would make us, the thesis team, proud to

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann marked 2 inline comments as done. mgartmann added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:16 + +This check implements `C.35

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann updated this revision to Diff 345635. mgartmann added a comment. Resolved readability-identifier-naming warning, adjusted check's documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102325/new/ https://reviews.llvm.org/D102325

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:22 +void VirtualBaseClassDestructorCheck::registerMatchers(MatchFinder *Finder) { + ast_matchers::internal::Matcher inheritsVirtualMethod = +

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann updated this revision to Diff 345623. mgartmann marked 2 inline comments as done. mgartmann added a comment. Incorporated Phabricator review feedback: - added matchers and tests for subclasses with inherited virtual methods - made aid methods static and not part of the check's class -

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann marked 17 inline comments as done. mgartmann added a comment. In D102325#2754241 , @njames93 wrote: > Whats the intended behaviour for derived classes and their destructors? Can > test be added to demonstrate that behaviour? If a derived

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

njames93 added a comment. Whats the intended behaviour for derived classes and their destructors? Can test be added to demonstrate that behaviour? Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:44 + "making it public

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:38 + + const auto Destructor = MatchedClassOrStruct->getDestructor(); + Please don't use auto unless type is spelled in same

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

mgartmann created this revision. mgartmann added reviewers: aaron.ballman, njames93, alexfh. mgartmann added a project: clang-tools-extra. Herald added subscribers: shchenz, kbarton, xazax.hun, mgorny, nemanjai. mgartmann requested review of this revision. Finds base classes and structs whose