nridge marked 3 inline comments as done. nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/XRefs.cpp:139 +// that constructor. FIXME(nridge): this should probably be handled in +// targetDecl() itself. +const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) { ---------------- I would appreciate some tips on how we could handle this in `targetDecl()`. Please see more specific comments below. ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:141 +const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) { + if (const VarDecl *VD = N->ASTNode.get<VarDecl>()) { + if (const Expr *Init = VD->getInit()) { ---------------- This part is intended to handle case #8 in `LocateSymbol.Ambiguous`, where you have a declaration of the form `Foo abcd("asdf");` which invokes a constructor. A naive attempt to add this logic to `TargetFinder::add(const Decl*)` resulted in the constructor being a target even for //references// to the variable, not just its declaration. ================ Comment at: clang-tools-extra/clangd/XRefs.cpp:147 } + while ((N = N->Parent)) { + if (N->ASTNode.get<CallExpr>() || N->ASTNode.get<ImplicitCastExpr>() || ---------------- 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? 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