hokein 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));
----------------
nridge wrote:
> 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).
Thinking more about this.
I'm aligned on the idea of only traversing the immediately declared constraint
since the TypeContraint is a wrapper of it. The regression is that we'd not
perform traversal if the immediately declared constraint is nullptr (this just
happens for broken code).
Fix ideas:
- build a recovery expression if we can't not build a immediately declared
constraint due to the missing required template arguments etc;
- still traverse the TC if immediately declared constraint is nullptr;
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
