[PATCH] D82739: [clangd] Improve heuristic resolution of dependent types in TargetFinder
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 13 02:24:17 PDT 2020
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
More information about the cfe-commits
mailing list