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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
