[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 a comment.
This revision now requires changes to proceed.

A few more nits.



================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:114-117
+  auto ParentIter = Parents.begin();
+  std::string ParentsStr = "'" + getNameAsString(*ParentIter) + "'";
+  for (++ParentIter; ParentIter != Parents.end(); ++ParentIter)
+    ParentsStr += " or '" + getNameAsString(*ParentIter) + "'";
----------------
I suggest using 1. range for loop; 2. std::string::append instead of operator+/operator+=. Not that performance is extremely important here, but it's also cleaner, IMO:

```
std::string ParentsStr = "'";
for (const CXXRecordDecl *Parent : Parents) {
  if (ParentsStr.size() > 1)
    ParentsStr.append("' or '");
  ParentsStr.append(getNameAsString(Parent));
}
ParentsStr.append("'");
```


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:126
+                   "in subclass%1; did you mean %2?")
+              << (MatchedDecl->getCalleeDecl()->getAsFunction())
+              << (Parents.size() > 1 ? "es" : "") << ParentsStr;
----------------
Please remove the parentheses.


================
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(); }
----------------
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'?


https://reviews.llvm.org/D44295