[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Haojian Wu via Phabricator via cfe-commits]

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