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!

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to