[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:128
+      diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+           "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+      << CalleeName << ParentsStr;
----------------
zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > 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?"
> > > "is not defined by a direct base class" is not a correct proposal. Issues that this check finds are all about calling a grand-parent's method instead of parent's whether base class defines this method or not.
> > > 
> > > How about 
> > > 
> > > > Qualified function name %0 refers to a function not from a direct base class; did you mean %1?
> > > 
> > > ?
> > Not keen on "from", but we could use `"qualified function name %q0 refers to a function that is not explicitly declared by a direct base class; did you mean %1?"`
> Changed to  
> 
> > qualified name %0 refers to a member which was overridden in subclass%1"
> 
> 
Please use `%q0` rather than calling your custom qualified name function. The diagnostics engine will handle printing the qualified name appropriately for you.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:39
+                                           const CXXMethodDecl &MemberDecl) {
+  BasesVector result;
+  for (auto &Base : ThisClass.bases()) {
----------------
Local variables should be capitalized.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:40-41
+  BasesVector result;
+  for (auto &Base : ThisClass.bases()) {
+    auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+    const auto *ActualMemberDecl =
----------------
Please add the `const` qualifier to both of these.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:42-43
+    auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+    const auto *ActualMemberDecl =
+        MemberDecl.getCorrespondingMethodInClass(BaseDecl);
+    if (!ActualMemberDecl)
----------------
Do not use `auto` here.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:49-53
+    auto *TypePtr =
+        ActualMemberDecl->getThisType(ActualMemberDecl->getASTContext())
+            .getTypePtr();
+    assert(TypePtr);
+    auto *Type = TypePtr->getPointeeCXXRecordDecl();
----------------
Do not use `auto` here as the type is not explicitly spelled out in the initialization. Also, please do not name the local variable `Type` as 1) it seem to be a declaration, and 2) `Type` is a type name.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:54
+    auto *Type = TypePtr->getPointeeCXXRecordDecl();
+    assert(Type);
+    if (Type->getCanonicalDecl()->isDerivedFrom(&GrandParent))
----------------
Please add a message to your asserts so that if they trigger, the user gets something more helpful as the diagnostic. Same elsewhere.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:89
+
+  auto *Member = Result.Nodes.getNodeAs<MemberExpr>("member");
+  assert(Member);
----------------
Please put appropriate explicit qualifiers on all of the `auto` uses (here and elsewhere).


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:93
+  auto *MemberDecl = dyn_cast<CXXMethodDecl>(Member->getMemberDecl());
+  assert(MemberDecl);
+
----------------
Instead of `dyn_cast` followed by `assert()`, you should just use `cast<>`, but only if you're certain you cannot get anything other than a `CXXMethodDecl` out of the expression.


================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:98
+
+  auto *ThisType = ThisTypePtr->getPointeeCXXRecordDecl();
+  assert(ThisType);
----------------
Do not use `auto` here.


================
Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:6
+
+Detects and fixes calls to grand-...parent virtual methods instead of calls
+to parent's virtual methods.
----------------
This formulation is a bit odd because of the `grand-...parent` part. I would reword to be more along the lines of: detects calls to qualified function names that refer to a function that is not explicitly declared by a direct base class.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295