mgartmann marked 17 inline comments as done.
mgartmann added a comment.
In D102325#2754241 <https://reviews.llvm.org/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 class inherits a `virtual` method from a base class, according to
guideline C.35, it is a potential base class as well.
Therefore, such a derived class gets flagged by the check if its destructor is
not specified correctly.
In order to flag derived classes that only inherit a `virtual` method but do
not override it, the following matcher had to be added:
ast_matchers::internal::Matcher<CXXRecordDecl> inheritsVirtualMethod =
hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
Test cases were added to demonstrate this behaviour.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:129
+
+AccessSpecDecl *VirtualBaseClassDestructorCheck::getPublicASDecl(
+ const CXXRecordDecl &StructOrClass) const {
----------------
njames93 wrote:
> Can be made static again. Also should probably return a `const AccessSpecDecl
> *`
@njames93 If I get this correctly, only `check` and `addMatcher` should be
member functions of a check's class. All the other aid methods should be static
and not part of the class.
Did I get this right?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:55
+
+.. option:: IndentationWidth
+
----------------
Eugene.Zelenko wrote:
> Shouldn't `Clang-format` take care about such things?
As @njames93 also suggested leaving the indentation to `clang-format`, I
removed this option and the indentation functionality.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102325/new/
https://reviews.llvm.org/D102325
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
