nridge marked an inline comment as done. nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/XRefs.cpp:147 } + while ((N = N->Parent)) { + if (N->ASTNode.get<CallExpr>() || N->ASTNode.get<ImplicitCastExpr>() || ---------------- 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.) 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