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