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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:24-25
+                       const CXXRecordDecl *ThisClass) {
+  assert(Parent);
+  assert(ThisClass);
+  if (Parent->getCanonicalDecl() == ThisClass->getCanonicalDecl())
----------------
You can drop these asserts.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:28-29
+    return true;
+  const auto ClassIter = std::find_if(
+      ThisClass->bases_begin(), ThisClass->bases_end(), [=](auto &Base) {
+        assert(Base.getType()->getAsCXXRecordDecl());
----------------
You can use `llvm::find_if()` for the range-based API instead.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30-32
+        assert(Base.getType()->getAsCXXRecordDecl());
+        return Parent->getCanonicalDecl() ==
+               Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl();
----------------
Factor out `Base.getType()->getAs()` and use the pointer in both the assert and the comparison.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:39-40
+                                const CXXRecordDecl *ThisClass) {
+  assert(Parent);
+  assert(ThisClass);
+  return ThisClass->isDerivedFrom(Parent);
----------------
And these asserts as well.

I don't think this function adds much value given that it's a one-liner; I'd hoist its functionality out to the caller.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:56-60
+  }
+  return result;
+}
+
+static std::string GetNameAsString(const NamedDecl *Decl) {
----------------
This should be `getNameAsString()`


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:101-103
+  else {
+    if (auto *TemplateSpecializationTypePtr =
+            Result.Nodes.getNodeAs<TemplateSpecializationType>("castToType")) {
----------------
Convert this to an `else if` and elide the braces.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:112
+
+  const auto Parents = GetParentsByGrandParent(CastToType, ThisType);
+  assert(!Parents.empty());
----------------
Do not use `auto` here as the type is not spelled out in the intialization.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+      diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+           "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+      << CalleeName << ParentsStr;
----------------
The diagnostic should not contain punctuation aside from the question mark.

Also, the diagnostic uses some novel terminology in "grandparent's method". How about: "qualified function name %0 refers to a function that is not defined by a direct base class; did you mean %1?"


================
Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:127
+// Just to instantiate DF<F>.
+int bar() { return (new DF<int>())->virt_1(); }
----------------
What should happen in this case?
```
struct Base {
  virtual void f() {}
};

struct Intermediary : Base {
};

struct Derived : Intermediary {
  void f() override {
    Base::f(); // I would expect this not to diagnose
  }
};
```
This would be a reasonable test case to add.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295