sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/FindTarget.cpp:720 + // Pick up the name via VisitNamedDecl + Base::VisitTemplateTypeParmDecl(D); + } ---------------- kadircet wrote: > nridge wrote: > > Am I using the API correctly here? It's a bit weird that `ConstDeclVisitor` > > would work differently from `RecursiveASTVisitor` in that VisitXXX methods > > for super-classes are not automatically called > right, non-recursive ast visitors are mere helpers for visiting most specific > type of an ast node which is interesting to the visitor (i.e. visit method is > overridden), and only that. they also don't traverse children of an entitie's > entries. > > which usually hints towards this being the wrong place to handle such > constructs. we actually have an outer recursiveastvisitor, that tries to > visit each children. but concept references seem to be missing from there. > taking a look at the RecursiveASTVisitor pieces around this piece, we > actually do visit the children appropriately: > - > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L1931 > calls traverse on the constraint > - > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L1923 > jumps into type constraint > - > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L510 > some more indirection > - > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L547 > calls visit for the decl name > - > https://github.com/llvm/llvm-project/blob/clang/include/clang/AST/RecursiveASTVisitor.h#L830 > unfortunately we stop here, because all we store is an identifier. > > We should probably figure out how to properly visit type constraints inside > the RecursiveASTVisitor (i guess we should actually be visiting > TypeConstraints). @usaxena95 who's looking at C++20 features and their > interactions with source tooling. > > In the meanwhile, i think we should have this specialization at the outer > layer, in `ExplicitReferenceCollector`, with something like: > ``` > bool TraverseTemplateTypeParamDeclConstraints(TemplateTypeParmDecl *TTPD) { > if (auto *TC = TTPD->getTypeConstraint()) { > reportReference(ReferenceLoc{TC->getNestedNameSpecifierLoc(), > TC->getConceptNameLoc(), > /*IsDecl=*/false, > {TC->getNamedConcept()}}, > DynTypedNode::create(*TTPD)); > return RecursiveASTVisitor::TraverseTypeConstraint(TC); > } > return true; > } > ``` > https://github.com/llvm/llvm-project/blob/clang/include/clang/AST/RecursiveASTVisitor.h#L830 > unfortunately we stop here, because all we store is an identifier. This isn't unfortunate, this is where we would handle things internal to the name (e.g. if the name was `operator vector<int>()`) rather than what the name is used for. The right level to handle this seems to be a missing `TraverseConceptReference()` which observes the lexical reference to a concept (we don't care here that it's a type constraint specifically). Unfortunately this doesn't exist: we have `TraverseConceptReferenceHelper()` which is private and called nonpolymorphically. Renaming this to `TraverseConceptReference()` and making it CRTP-overridable seems like the principled solution at the RAV level. Maybe there's some reason it wasn't done this way to start with though. Failing that, overriding `TraverseTypeConstraint()` in our RAV subclass seems like it fits the pattern more neatly (there's no VisitTypeConstraint, so we have to override Traverse and call Base::Traverse). We're going to have to handle the other ways concepts can be referenced of course, but changing RAV vs working around its limitations is always a tradeoff of practicality. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142871/new/ https://reviews.llvm.org/D142871 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
