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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: kbobyrev.

thanks, looks good.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:71
+
+  if (const auto *RT = T->getAs<RecordType>()) {
+    return dyn_cast<CXXRecordDecl>(RT->getDecl());
----------------
nit: remove the `{}`, and elsewhere.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:91
+// Try to heuristically resolve the type of a dependent expression `E`.
+const Type *resolveDependentExprToType(const Expr *E) {
+  std::vector<const NamedDecl *> Decls = resolveDependentExprToDecls(E);
----------------
I'd move `resolveDependentExprToType` close to `resolveDependentExprToDecls` (they are quite related).

consider putting `resolveDependentExprToType` after `resolveDependentExprToDecls` definition, and add a forward declaration of `resolveDependentExprToType` before the `resolveDependentExprToDecls`.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:190
     }
+    Expr *Base = ME->isImplicitAccess() ? nullptr : ME->getBase();
+    if (const auto *BT = BaseType->getAs<BuiltinType>()) {
----------------
nit: move the Base to the if below.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:194
+      // represented as BultinType::Dependent which gives us no information. We
+      // can get further b analyzing the depedent expression.
+      if (Base && BT->getKind() == BuiltinType::Dependent) {
----------------
nit: b -> by


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:197
+        BaseType = resolveDependentExprToType(Base);
+        if (!BaseType)
+          return {};
----------------
this branch is redundant, we can remove it, since `getMembersReferencedViaDependentName` can handle a nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82739