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:
> > 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?"`
================
Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:107-108
+
+ std::string CalleeName =
+ getNameAsString(MatchedDecl->getCalleeDecl()->getAsFunction());
+ assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid());
----------------
You can get rid of this and just pass in the `Decl *` directly while using `%q`
in the format string.
================
Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+ int virt_1() override { return A::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'?
[bugprone-parent-virtual-call]
+ // CHECK-FIXES: int virt_1() override { return BI::virt_1(); }
----------------
zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > This seems like a false positive to me. Yes, the virtual function is
> > > > technically exposed in `BI`, but why is the programmer obligated to
> > > > call that one rather than the one from `A`, which is written in the
> > > > source?
> > > IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but
> > > tomorrow someone will implement BI::virt_1() and it will silently lead to
> > > bugs or whatever.
> > If tomorrow someone implements `BI::virt_1()`, then the check will start
> > diagnosing at that point.
> Correct, but anyway I don't think it's a problem.
I'd prefer to see this changed to not diagnose. I don't relish the idea of
explaining why that code diagnoses as it stands.
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
