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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:110-111
+    diag(MatchedClassOrStruct->getLocation(),
+         "destructor of %0 is private and prevents using the type. Consider "
+         "making it public and virtual or protected and non-virtual")
+        << MatchedClassOrStruct;
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:135-137
+       "destructor of %0 is %select{public and non-virtual|protected and "
+       "virtual}1. It should either be public and virtual or protected and "
+       "non-virtual")
----------------
Same issue here as above.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-base-class-destructor.cpp:8
+private:
+  virtual ~PrivateVirtualBaseStruct(){};
+};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325