nridge added inline comments.
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1843
+ if (const auto *TC = D->getTypeConstraint()) {
+ TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
TRY_TO(TraverseConceptReference(*TC));
----------------
hokein wrote:
> nridge wrote:
> > hokein wrote:
> > > Looks like we may visit some nodes in `ConceptReference` twice:
> > > - getImmediatelyDeclaredConstraint returns a `ConceptSpecializationExpr`
> > > (most cases?) which is a subclass of `ConceptReference`;
> > > - `TraverseStmt(ConceptSpecializationExpr*)` will dispatch to
> > > `TraverseConceptSpecializationExpr` which invokes
> > > `TraverseConceptReference` (see Line 2719);
> > >
> > >
> > > It is sad that we don't have enough test coverage, could you write some
> > > tests in `clang/unittests/Tooling/RecursiveASTVisitorTests/`?
> > It is true that there will be two calls to `TraverseConceptReference()`.
> > However, they are called on two different `ConceptReference` objects:
> >
> > * the call in `TraverseConceptSpecializationExpr` will visit the base
> > subobject of the `ConceptSpecializationExpr` (which inherits from
> > `ConceptReference`)
> > * the call in `TraverseTemplateTypeParmDecl` will visit the base
> > subobject of the `TypeConstraint` (which also inherits from
> > `ConceptReference`).
> >
> > So, I think this is fine -- there are two distinct `ConceptReference`
> > objects in the AST, and with this patch we visit both of them.
> I understand that they are two different `ConceptReference` objects, but they
> have members (`FoundDecl`, `ArgsAsWritten`) that may refer to the same AST
> nodes.
>
> ```
> template <typename T, typename U>
> concept binary_concept = true;
> struct Foo {};
>
> template<binary_concept<Foo> T> // the template argument Foo will be visited
> twice.
> void k2();
> ```
>
> I'm not sure what's is the right approach here, I can see two options:
>
> - traverse TC + immediately-declared-constraint expr, this seem to cause some
> ast nodes visited twice (maybe not a big deal?)
> - just traverse immediately-declared-constraint expr, this seems not breaking
> any tests, but the immediately-declared-constraint expr could be nullptr
> (e.g. broken code, missing required template arguments); or the
> immediately-declared-constraint expr could be a `CXXFoldExpr`, which will
> make some members in `ConceptReference` not be visited;
>
> @rsmith, do you have any idea about this?
>
From clangd's point of view, it would be sufficient to visit the
immediately-declared-constraint-expr without visiting any of its descendants.
However, I'm not sure how to accomplish this using `RecursiveASTVisitor`. (I
think I'd want to call `WalkUpFromXXX(TC->getImmediatelyDeclaredConstraint())`,
where `XXX` is the dynamic type of the immediately-delcared-constraint, but I
don't know how to dispatch to that dynamic type; `RecursiveASTVisitor` seems to
be designed to do the dispatch on `Traverse` calls, not `WalkUpFrom` calls).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84136/new/
https://reviews.llvm.org/D84136
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
