[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:73
+// 'typedef'ed grand-parent classes.
+// Adopted from https://stackoverflow.com/a/23081770/1073080
+static std::string getExprAsString(const clang::Expr &E,
----------------
That thread doesn't seem to be particularly useful. `Lexer::getSourceText(CharSourceRange::getTokenRange(E.getSourceRange()), SM, CorrectLangOpts, 0);` (or `clang::tooling::fixit::getText`) should work well in macro-free cases. When there are macros, `Lexer::makeFileCharRange` may help to find a contiguous range that corresponds to a `SourceRange` (and when it doesn't, this usually means that there's no such text range).

Both of these don't remove whitespace, but why would you do that anyway? If really needed, this can be done outside.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77
+  std::string Text = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(E.getSourceRange()), SM, LangOptions(), 0);
+  // Strip the string.
----------------
Any reason to not use the actual LangOptions? Substituting it with the default may result in incorrect handling of certain tokens.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:79
+  // Strip the string.
+  Text.erase(std::remove_if(begin(Text), end(Text), std::isspace), end(Text));
+  if (!Text.empty() && (Text.at(Text.size() - 1) == ','))
----------------
Consider using `llvm::remove_if`.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:80
+  Text.erase(std::remove_if(begin(Text), end(Text), std::isspace), end(Text));
+  if (!Text.empty() && (Text.at(Text.size() - 1) == ','))
+    return Lexer::getSourceText(
----------------
Text.at(Text.size() - 1) seems to be the same as Text.back(). And what does this condition do, btw?


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:81-83
+    return Lexer::getSourceText(
+        CharSourceRange::getCharRange(E.getSourceRange()), SM, LangOptions(),
+        0);
----------------
That seems to be a bad thing to fall back to - it uses getCharRange which will drop the last token.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:144-146
+  // auto Diag = diag(MatchedDecl->getSourceRange().getBegin(), "variable
+  // declared here")
+  //    << MatchedDecl->getSourceRange();
----------------
Please remove the commented out code.


================
Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:32
+  int virt_2() override { return A::virt_1() + B::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return B::virt_1() + B::virt_1(); }
----------------
alexfh wrote:
> Specifying each unique message completely once should be enough to detect typos in it (e.g. missing or extra spaces or quotes around placeholders). In other messages let's omit repeated static parts to make the test a bit more easier to read, e.g.:
> 
>   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'B'?
I'd also remove the ` [bugprone-parent-virtual-call]` part from repeated CHECK-patterns.


https://reviews.llvm.org/D44295