- Try to apply heuristic resolution recursively to the base expression of a CXXDependentScopeMemberExpr.
- Try to apply heuristic resolution recursively to the callee expression in a call expression.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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).
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
254 | 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, 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.
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
254 | 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. |
thanks, this sounds a good plan.
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
171–172 | 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(); } }; | |
254 | it looks better now, thanks! |
Address review comments
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
171–172 | Thank you for the example! Handling this case required a bit of additional logic, which I've now added. |
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
71 | 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, | |
87 | nit: the "a dependent type" doesn't seem to be correct, the type is not always dependent, I'd use a tag-decl type. | |
101 | 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. | |
197 | 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; } }; | |
198 | 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; } }; | |
199 | could you add a unittest for this case? nit: I'd remove the extra {} if the if body just contains a single statement. | |
287 | this FIXME looks stale now, could you please update it? |
Address review comments
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
197 | Thanks for the additional example. To make this work I had to handle one more case (MemberExpr) in resolveDependentExprToDecls(). | |
287 | I think the FIXME is still relevant. While we handle DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr, we still do not handle the AST node types listed in the comment. For example, we do not handle: template <typename> struct A { typedef int [[type]]; }; template <typename T> struct B { typedef typename A<T>::t^ype type; }; (I believe that one would be DependentNameType? You can imagine similar scenarios involving using-decls etc.) However, the example in the FIXME (which depicts a CXXDependentScopeMemberExpr) is now stale, so I've removed it and clarified the comment a bit. |
thanks, looks good.
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
67 | nit: remove the {}, and elsewhere. | |
88 | 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. | |
178 | nit: move the Base to the if below. | |
182 | nit: b -> by | |
185 | this branch is redundant, we can remove it, since getMembersReferencedViaDependentName can handle a nullptr. |
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
172 | @nridge, the assertion is not true anymore, since we have extended it to support possibly-non-dependent expressions (callExpr, MemberExpr). we hit this assert when opening clang/include/clang/ASTMatchers/ASTMatchers.h. I think we probably need to remove it and rename related function names. |
clang-tools-extra/clangd/FindTarget.cpp | ||
---|---|---|
172 | a reduce case: template<typename T> class Foo { public: int foo(); }; class Bar { public: static void k(); template <typename T> T convert() const; }; void func(); template <typename T> void foo2(Foo<T> t) { Bar::k(t.foo()).template convert<T>(); } |
nit: remove the {}, and elsewhere.