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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Have you tried running this over any large C++ code bases to see how often the diagnostics trigger and whether there are false positives? If not, you should probably do so -- one concern I have is with a private virtual destructor; I could imagine people using that for a class that is intended to be created but not destroyed (like a singleton).



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:99
+    CheckFactories.registerCheck<VirtualBaseClassDestructorCheck>(
+        "cppcoreguidelines-virtual-base-class-destructor");
   }
----------------
I think this may be a bit confusing of a name for the check -- this suggests to me it's about virtual base classes and their destructors, but the check is really more about defining a destructor properly in a class used as a base class with a vtable. However, this check follows the enforcement from the rule rather than the rule wording itself -- it doesn't care whether the class is ever used as a base class or not, it only cares whether the class contains a virtual function. How about: `cppcoreguidelines-virtual-class-destructor`? (Probably worth it to rename the check at the same time to keep the public check name and the internal check implementation names consistent.)


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:28
+      cxxRecordDecl(
+          anyOf(isStruct(), isClass()),
+          anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
----------------
I believe you can remove this without changing the behavior -- unions can't have virtual member functions anyway.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:80
+      Loc = StructOrClass.getEndLoc();
+      DestructorString.append("public:");
+      AppendLineBreak = true;
----------------



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:89-93
+  DestructorString.append("\n")
+      .append("virtual ~")
+      .append(StructOrClass.getName().str())
+      .append("() = default;")
+      .append(AppendLineBreak ? "\n" : "");
----------------
This should be using an `llvm::Twine` (https://llvm.org/docs/ProgrammersManual.html#the-twine-class).


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:117-121
+      std::string()
+          .append(Visibility)
+          .append(":\n")
+          .append(Visibility == "public" && !Destructor.isVirtual() ? "virtual "
+                                                                    : "");
----------------
This function should also be using a Twine for much of this -- basically, Twine helps you build up a string from various components (some `std::string`, some `StringRef`, some `const char *`, etc) in an efficient manner that only does memory allocation when needing the full string contents.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:123-125
+  if (Visibility == "protected" && Destructor.isVirtualAsWritten()) {
+    OriginalDestructor = eraseKeyword(OriginalDestructor, "virtual ");
+  }
----------------



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:135-141
+  if (Destructor.isExplicitlyDefaulted()) {
+    EndLocation =
+        utils::lexer::findNextTerminator(Destructor.getEndLoc(), SM, LangOpts)
+            .getLocWithOffset(1);
+  } else {
+    EndLocation = Destructor.getEndLoc().getLocWithOffset(1);
+  }
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:6-8
+Finds base classes and structs whose destructor is neither public and virtual
+nor protected and non-virtual. A base class's destructor should be specified in
+one of these ways to prevent undefined behaviour.
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:24-50
+  struct Foo {  // NOK, protected destructor should not be virtual
+    virtual void f();
+  protected:
+    virtual ~Foo(){};
+  };
+
+  class Bar {    // NOK, public destructor should be virtual
----------------
There are a bunch of trailing semicolons in the example code that can be removed.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp:6
+// CHECK-MESSAGES: :[[@LINE+2]]:8: note: make it protected
+// As we have 2 conflicting fixes in notes, no fix is applied.
+struct PrivateVirtualBaseStruct {
----------------
Thank you for the comment about the fix not being applied!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325