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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:76
   if (const auto *ICNT = T->getAs<InjectedClassNameType>()) {
     T = ICNT->getInjectedSpecializationType().getTypePtrOrNull();
   }
----------------
an off-topic comment: I don't have a reproduce case, but I think we might get a nullptr, and get a null-access crash below,


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:101
+
 // Given a dependent type and a member name, heuristically resolve the
 // name to one or more declarations.
----------------
nit: the "a dependent type" doesn't seem to be correct, the type is not always dependent, I'd use `a tag-decl type`.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:119
 std::vector<const NamedDecl *> getMembersReferencedViaDependentName(
-    const Type *T,
+    Expr *E, const Type *T,
     llvm::function_ref<DeclarationName(ASTContext &)> NameFactory,
----------------
this function was pretty clear prior to the patch -- given a tag-decl type, and a member name, it tries to perform a lookup in the tag-decl, and returns the result.

Now we add an extra&optional `E` which seems making it tangled, there is only one caller passing the actual `E`, so I think we should keep this helper function as-is, and handle the builtin-dependent type in the caller.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:214
   }
+  if (const auto *CE = dyn_cast<CallExpr>(E)) {
+    const auto *CalleeType = resolveDependentExprToType(CE->getCallee());
----------------
btw, could you try the case below? it doesn't seem to work.

```
struct Bar {
  int aaaa;
};
template <typename T> struct Foo {
  Bar func(int);
  void test() {
    func(1).aa^aa;
  }
};
```


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:215
+  if (const auto *CE = dyn_cast<CallExpr>(E)) {
+    const auto *CalleeType = resolveDependentExprToType(CE->getCallee());
+    if (const auto *FnTypePtr = CalleeType->getAs<PointerType>()) {
----------------
we need to check whether the type is null, otherwise we get a crash below.

see:

```
   template<typename T>
   struct Foo {
     int func(int);
     void test() {
       func().a;
     }
   };

```




================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:216
+    const auto *CalleeType = resolveDependentExprToType(CE->getCallee());
+    if (const auto *FnTypePtr = CalleeType->getAs<PointerType>()) {
+      CalleeType = FnTypePtr->getPointeeType().getTypePtr();
----------------
could you add a unittest for this case?

nit: I'd remove the extra `{}` if the if body just contains a single statement. 


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:287
 //
 // FIXME: improve common dependent scope using name lookup in primary templates.
 // e.g. template<typename T> int foo() { return std::vector<T>().size(); }
----------------
this FIXME looks stale now, could you please update it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739