5chmidti wrote:
> for me oryginal version that used registerMatchers instead of visitor were
> way better, specially that there are matchers that could be used for this, or
> could be written.
We do have the `isDerivedFrom` matcher, actually.
> in most projects not many classes actually used std::enable_shared_from_this
> and there will be no visible performance gain from "remembering" list of
> classes, and as we could see this approach makes this check more complicated.
Maybe we should just go with what is simpler, and check the performance of the
implementation, but I think it could be more performant.
```c++
struct A {};
struct B : public A {};
struct C : public B {};
struct D : public B {};
```
When we are visiting a definition of a class, we know that every base class has
been visited already, so we do not need to traverse the inheritance hierarchy
and instead just need to check our set.
We mark `A` as either an `enable_shared_from_this` class, or not.
We check for `B`, if any of its base classes is already in the set, and the
same goes for `C` and `D`. The base classes of `C` and `D` are never actually
transitively traverse when we check `C` and `D`, but instead we do a lookup
because we already computed the answer.
---
@MichelleCDjunaidi
Untested, but this should be roughly what needs to be done (minus the check for
a missing `std::` in front of `enable_shared_from_this`).
```c++
bool VisitCXXRecordDecl(CXXRecordDecl *RDecl) {
if (RDecl->getQualifiedNameAsString() ==
"std::enable_shared_from_this")
EnableSharedClassSet.insert(RDecl->getCanonicalDecl());
for (const auto &Base : RDecl->bases()) {
const auto* BaseRecord =
Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl();
if (EnableSharedClassSet.contains(BaseRecord))
{
if (Base.getAccessSpecifier() != AccessSpecifier::AS_public)
Check.diag(...);
else
EnableSharedClassSet.insert(RDecl->getCanonicalDecl());
}
}
return true;
}
```
Your `VisitCXXBaseSpecifier` has the same name as the other visitor functions,
but it is technically not in the format of the `RecursiveASTVisitor` and may be
confusing. `Handle...` or `Check...` would be better IMO.
Either way, the direction is still an open question
https://github.com/llvm/llvm-project/pull/102299
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
