Page MenuHomePhabricator

[clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check
Needs ReviewPublic

Authored by mgartmann on May 12 2021, 6:53 AM.

Details

Summary

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.

Fixes are available for user-declared and implicit destructors that are either public
and non-virtual or protected and virtual.

This check implements C.35 from the CppCoreGuidelines.

Diff Detail

Event Timeline

mgartmann created this revision.May 12 2021, 6:53 AM
mgartmann requested review of this revision.May 12 2021, 6:53 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
38 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

77 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

78 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

83 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

96 ↗(On Diff #344806)

Why not just std::string DestructorString?

101 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

105 ↗(On Diff #344806)

Please don't use auto unless type is spelled in same statement or iterator.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
9 ↗(On Diff #344806)

Links to documentation usually placed at the end. Please also try to follow 80 characters line length limit

16 ↗(On Diff #344806)

Is it really necessary? Same below.

55 ↗(On Diff #344806)

Shouldn't Clang-format take care about such things?

Whats the intended behaviour for derived classes and their destructors? Can test be added to demonstrate that behaviour?

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
44 ↗(On Diff #344806)

Printing class or struct here isn't necessary, but if you really feel it should be included use %select{class|struct}0 instead.

51 ↗(On Diff #344806)

This can no be made a bool, see below for the Diag.

68 ↗(On Diff #344806)
70 ↗(On Diff #344806)

Again this can be removed.

74 ↗(On Diff #344806)

This doesn't need to be part of the class and should just be a static function in this file.

101 ↗(On Diff #344806)

Again, don't worry about indentation, clang-format does that.

129 ↗(On Diff #344806)

Can be made static again. Also should probably return a const AccessSpecDecl *

143–145 ↗(On Diff #344806)

As clang-format handles indentation, we can just remove this function.

mgartmann marked 17 inline comments as done.May 15 2021, 4:10 AM

Whats the intended behaviour for derived classes and their destructors? Can test be added to demonstrate that behaviour?

If a derived class inherits a virtual method from a base class, according to guideline C.35, it is a potential base class as well.
Therefore, such a derived class gets flagged by the check if its destructor is not specified correctly.

In order to flag derived classes that only inherit a virtual method but do not override it, the following matcher had to be added:

ast_matchers::internal::Matcher<CXXRecordDecl> inheritsVirtualMethod =
    hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));

Test cases were added to demonstrate this behaviour.

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
129 ↗(On Diff #344806)

@njames93 If I get this correctly, only check and addMatcher should be member functions of a check's class. All the other aid methods should be static and not part of the class.

Did I get this right?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
55 ↗(On Diff #344806)

As @njames93 also suggested leaving the indentation to clang-format, I removed this option and the indentation functionality.

mgartmann updated this revision to Diff 345623.May 15 2021, 4:18 AM
mgartmann marked 2 inline comments as done.

Incorporated Phabricator review feedback:

  • added matchers and tests for subclasses with inherited virtual methods
  • made aid methods static and not part of the check's class
  • replaced auto with types where it was suggested
  • adjusted diagnostic messages

Included new commits of upstream main branch to resolve build failure.

Eugene.Zelenko added inline comments.May 15 2021, 5:57 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
22 ↗(On Diff #345623)

Please fix readability-identifier-naming warning.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
16 ↗(On Diff #345623)

Links to documentation usually placed at the end. See other checks documentation.

mgartmann updated this revision to Diff 345635.May 15 2021, 7:24 AM

Resolved readability-identifier-naming warning, adjusted check's documentation.

mgartmann marked 2 inline comments as done.May 15 2021, 7:30 AM
mgartmann added inline comments.
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
16 ↗(On Diff #345623)

Thanks for pointing this out to me!
I previously followed the style of other cppcoreguideline checks which did not have this line at the end of the file (e.g., cppcoreguidelines-prefer-member-initializer or cppcoreguidelines-avoid-non-const-global-variables.

I adjusted the documentation according to your suggestion and now placed this line at the end.

mgartmann marked an inline comment as done.Mon, May 24, 8:23 AM

Dear reviewers,

Since this work was conducted as part of a Bachelor's thesis, which has to be
handed in on the 18th of June, 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Fabian & Marco

FWIW, this looks like it's also partially covering -Wnon-virtual-dtor, but that doesn't seem to have the checks for a protected destructor.

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
110–111 ↗(On Diff #345635)

clang-tidy diagnostics should not contain full sentences with punctuation in them (they follow the Clang diagnostic rules).

One thing you could do to resolve this is to issue one diagnostic for the warning, and two diagnostics for notes with fixits attached to them. e.g.,

warning: destructor of %0 is 'private' and prevents using the type
note 1: consider making it public and virtual (fixit to insert public: before the declaration, insert virtual in the declaration,
and insert private: after the declaration)
note 2: consider making it protected (fixit to insert protected: before the declaration and insert private: after the declaration)

If you don't want to do the fixits, you could reword the diagnostic to combine the sentences, but it's a bit lengthy.

135–137 ↗(On Diff #345635)

Same issue here as above.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp
8 ↗(On Diff #345635)

All of these empty dtors have a semi-colon after their closing brace that can be removed.

I'd also like to see a test that = default works fine for the dtor.

mgartmann marked 3 inline comments as done.Tue, Jun 1, 9:06 AM
mgartmann updated this revision to Diff 348988.Tue, Jun 1, 9:08 AM
  • added fixes for private destructors
  • separated fixes for private destructors into notes
  • added a test case for a = default; constructor

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).

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
99

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.)

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp
28 ↗(On Diff #348988)

I believe you can remove this without changing the behavior -- unions can't have virtual member functions anyway.

80 ↗(On Diff #348988)
89–93 ↗(On Diff #348988)
117–121 ↗(On Diff #348988)

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.

123–125 ↗(On Diff #348988)
135–141 ↗(On Diff #348988)
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst
6–8 ↗(On Diff #348988)
24–50 ↗(On Diff #348988)

There are a bunch of trailing semicolons in the example code that can be removed.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp
6 ↗(On Diff #348988)

Thank you for the comment about the fix not being applied!

mgartmann updated this revision to Diff 351869.Mon, Jun 14, 7:38 AM
mgartmann marked 10 inline comments as done.
  • changed name of check to cppcoreguidelines-virtual-class-destructor
  • removed matcher for class or struct in AST matcher
  • changed string concatenations to use llvm::twine
  • adjusted documentation and removed unnecessary semicolons in it

Thanks a lot for your feedback, @aaron.ballman!

I was able to incorporate it in the updated diff.

On your advice, I also ran this check on a large open source project using the run-clang-tidy.py script. I decided to do it on the llvm project itself as follows:

marco@nb:~/llvm-project/build$ python3 run-clang-tidy.py -checks="-*,cppcoreguidelines-virtual-class-destructor"  -header-filter=".*" > result.txt
marco@nb:~/llvm-project/build$ cat result.txt | grep "private and prevents" | wc -l
797

Most of these 797 lines are duplicates, since these warnings come from header files included in (apparently) many other files.
Deduplicated, the following seven destructors were private and triggered this check's warning:

  • llvm-project/llvm/include/llvm/Analysis/RegionInfo.h:1024:23: warning: destructor of 'RegionInfoBase<llvm::RegionTraits<llvm::Function>>' is private ...
  • llvm-project/llvm/include/llvm/Analysis/RegionInfo.h:674:7: warning: destructor of 'RegionInfoBase' is private ...
  • llvm-project/llvm/include/llvm/CodeGen/MachineRegionInfo.h:177:23: warning: destructor of 'RegionInfoBase<llvm::RegionTraits<llvm::MachineFunction>>' is private ...
  • llvm-project/llvm/lib/Target/AVR/MCTargetDesc/AVRMCExpr.h:19:7: warning: destructor of 'AVRMCExpr' is private ...
  • llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1257:18: warning: destructor of 'UnitTest' is private ...
  • llvm-project/llvm/lib/CodeGen/InlineSpiller.cpp:159:7: warning: destructor of 'InlineSpiller' is private ...
  • llvm-project/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:387:7: warning: destructor of 'X86MCInstrAnalysis' is private ...

I assume that these destructors are private by on purpose. Please correct me if I am wrong.
A programmer working on the LLVM project would therefore see seven warnings for private destructors over the whole project.

Is this number of private destructors which are flagged acceptable to you?