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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman edited reviewers, added: hokein, njames93; removed: aaron.ballman, alexfh_.
aaron.ballman edited subscribers, added: aaron.ballman; removed: llvm-commits.
aaron.ballman added a comment.

Removing myself as a reviewer (I've generally been stepping back from reviewing C++ Core Guideline checks given the continued lack of reasonable consideration for enforcement in the rules), but I did have some drive-by comments.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualDestructor =
+      hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual())))));
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:206
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
----------------
Rather than use a comment, we typically put the test cases into a namespace. e.g.,
```
namespace PR51912 {
// tests cases for that bug live here
} // namespace PR51912
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
----------------
I think there are more interesting template test cases that should be checked.
```
// What to do when the derived type is definitely polymorphic
// but the base class may or may not be?
template <typename Ty>
struct Derived : Ty {
  virtual void func();
};

struct S {};
struct  T { virtual ~T(); };

void instantiate() {
  Derived<S> d1; // Diagnose
  Derived<T> d2; // Don't diagnose
}
```



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:220
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
----------------



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

https://reviews.llvm.org/D110614