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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:38
+
+  const auto Destructor = MatchedClassOrStruct->getDestructor();
+
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:77
+    const LangOptions &LangOpts) const {
+  auto VirtualBeginLoc = Destructor.getBeginLoc();
+  auto VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:78
+  auto VirtualBeginLoc = Destructor.getBeginLoc();
+  auto VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
+      Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:83
+  /// virtual is included.
+  auto StartOfNextToken = Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
+                              .getValue()
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:96
+    const SourceManager &SourceManager) const {
+  auto DestructorString = std::string("");
+  SourceLocation Loc;
----------------
Why not just `std::string DestructorString`?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:101
+
+  auto ParentIndentation =
+      SourceManager.getExpansionColumnNumber(StructOrClass.getBeginLoc()) -
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualBaseClassDestructorCheck.cpp:105
+
+  auto AccessSpecDecl = getPublicASDecl(StructOrClass);
+
----------------
Please don't use auto unless type is spelled in same statement or iterator.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:9
+
+This check implements `C.35 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual>`_ from the CppCoreGuidelines.
+
----------------
Links to documentation usually placed at the end. Please also try to follow 80 characters line length limit


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:16
+  struct Foo {  // NOK, protected destructor should not be virtual
+         ~~~
+    virtual void f();
----------------
Is it really necessary? Same below.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-base-class-destructor.rst:55
+
+.. option:: IndentationWidth
+
----------------
Shouldn't `Clang-format` take care about such things?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325