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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
