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 and virtual or protected and non-virtual") + << (MatchedClassOrStruct->isClass() ? "class" : "struct") + << MatchedClassOrStruct; ---------------- Printing `class` or `struct` here isn't necessary, but if you really feel it should be included use `%select{class|struct}0` instead. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:51 + // Implicit destructors are public and non-virtual for classes and structs. + std::string TypeAndVirtuality = "public and non-virtual"; + FixItHint Fix; ---------------- This can no be made a bool, see below for the Diag. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:68 + diag(MatchedClassOrStruct->getLocation(), + "destructor of %0 %1 is %2. It should either be public and virtual or " + "protected and non-virtual") ---------------- ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:70 + "protected and non-virtual") + << (MatchedClassOrStruct->isClass() ? "class" : "struct") + << MatchedClassOrStruct << TypeAndVirtuality << Fix; ---------------- Again this can be removed. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:74 + +CharSourceRange VirtualBaseClassDestructorCheck::getVirtualKeywordRange( + const CXXDestructorDecl &Destructor, const SourceManager &SM, ---------------- This doesn't need to be part of the class and should just be a static function in this file. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:101 + + auto ParentIndentation = + SourceManager.getExpansionColumnNumber(StructOrClass.getBeginLoc()) - ---------------- Eugene.Zelenko wrote: > Please don't use auto unless type is spelled in same statement or iterator. Again, don't worry about indentation, clang-format does that. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:129 + +AccessSpecDecl *VirtualBaseClassDestructorCheck::getPublicASDecl( + const CXXRecordDecl &StructOrClass) const { ---------------- Can be made static again. Also should probably return a `const AccessSpecDecl *` ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:143-145 +std::string VirtualBaseClassDestructorCheck::indent(int Indentation) const { + return std::string().append(Indentation, ' '); +} ---------------- As clang-format handles indentation, we can just remove this function. 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