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