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