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

[Nathan James via Phabricator via cfe-commits]

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