nridge marked an inline comment as done.
nridge added a comment.
So, to summarize my understanding of the way forward here:
1. Land the refactoring without any constructor targeting (i.e. the current
patch, which is ready to review in this state).
2. In a follow-up patch: target constructor in cases where this can be
accomplished by having SelectionTree expose the CXXConstructorExpr on the
parenthesis.
3. Longer-term: put in place a mechanism for exposing all implicit names via
code lens or somesuch.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:147
}
+ while ((N = N->Parent)) {
+ if (N->ASTNode.get<CallExpr>() || N->ASTNode.get<ImplicitCastExpr>() ||
----------------
nridge wrote:
> sammccall wrote:
> > nridge wrote:
> > > This part is a super hacky way to keep case #9 in
> > > `LocateSymbol.Ambiguous` passing while not regressing other tests.
> > >
> > > In that case, the selection tree looks like this:
> > >
> > > ```
> > > DeclStmt Foo foox = Foo("asdf");
> > >
> > > VarDecl Foo foox = Foo("asdf")
> > > ExprWithCleanups Foo("asdf")
> > > CXXConstructExpr Foo("asdf")
> > > MaterializeTemporaryExpr Foo("asdf")
> > > CXXFunctionalCastExpr Foo("asdf")
> > > .RecordTypeLoc Foo
> > > ```
> > >
> > > The common ancestor is the `RecordTypeLoc`, but we'd like to find the
> > > constructor referred to by the `CXXConstructExpr` as a target as well. To
> > > achieve that, my current code walks up the parent tree until we encounter
> > > a `CXXConstructExpr`, but aborts the walk if certain node types are
> > > encountered to avoid certain other scenarios where we have a
> > > `CXXConstructExpr` ancestor.
> > >
> > > This is almost certainly wrong, as there are likely many other cases
> > > where we have a `CXXConstructExpr` ancestor but don't want to find the
> > > constructor as a target which are not being handled correctly.
> > >
> > > Is there a better way to identify the syntactic form at issue in this
> > > testcase?
> > This case is gross, but as above I think it would be OK to have the
> > targeting work like:
> > ```
> > Foo("asdf")
> > FFFCSSSSSSC
> >
> > F -> RecordTypeLoc -> CXXRecordDecl Foo
> > C -> CXXConstructExpr -> CXXConstructorDecl Foo(string)
> > S -> StringLiteral -> nullptr
> > ```
> > WDYT?
> In your description, the target nodes are written underneath the characters.
> However, our input here is a cursor position (in between two characters).
>
> Should we target the `CXXConstructExpr` if the cursor is just before the
> parenthesis, just after, or both?
>
> (I am aware that `SelectionTree` can take a range as well as a position, so
> from the point of view of that API, "only when the selection is a range
> covering the parenthesis" is an option as well. However, this would not
> benefit go-to-definition, for which the request as specified in LSP only
> contains a position, not a range.)
Ah, never mind, I see now that `SelectionTree` has existing logic for mapping a
cursor position to a one-character selection, which in this case would result
in mapping the cursor onto the parenthesis if it's just before the parenthesis.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69237/new/
https://reviews.llvm.org/D69237
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
