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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits