hokein added a comment. In D82739#2138260 <https://reviews.llvm.org/D82739#2138260>, @nridge wrote:
> In D82739#2133155 <https://reviews.llvm.org/D82739#2133155>, @hokein wrote: > > > looks like we use this heuristic for go-to-def, I think we might want to > > use it in more places, e.g. libindex (for xrefs), sema code completion (so > > that `this->a.^` can suggest something). > > > Yeah, this is a great point. I would definitely like to reuse this heuristic > resolution for other features like code completion (including in > https://github.com/clangd/clangd/issues/443 where I hope to build on it to > improve the original STR which involves completion). > > I will explore relocating this code to a place where things like code > completion can reuse it, in a follow-up patch. thanks, this sounds a good plan. ================ Comment at: clang-tools-extra/clangd/FindTarget.cpp:196 switch (E->getStmtClass()) { case Stmt::CXXDependentScopeMemberExprClass: { const auto *ME = llvm::cast<CXXDependentScopeMemberExpr>(E); ---------------- I'm doubting whether we will miss some other exprs, since we are using it to find the decls for the base expr of `CXXDependentScopeMemberExpr`. could you try the following testcase (also add it to the unittest)? ``` struct A { void foo() {} }; struct B { A getA(); }; template <typename T> struct C { C c; void bar() { this->c.getA().foo(); } }; ``` ================ Comment at: clang-tools-extra/clangd/FindTarget.cpp:198 + // analyzing it. + if (E && BT->getKind() == BuiltinType::Dependent) { + T = resolveDependentExprToType(E); ---------------- nridge wrote: > hokein wrote: > > I think this is the key point of the fix? > > > > It would be nice if you could find a way to split it into two (one for > > refactoring, one for the fix). The diff of this patch contains large chunk > > of refactoring changes which make the fix less obvious. > Yeah, sorry about that. I've split the patch and cleaned it up further (to > avoid unnecessary reordering of functions) to make the diff neater. it looks better now, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82739/new/ https://reviews.llvm.org/D82739 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits