sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Awesome, thanks, I'd been hoping to get around to this! LG with some
naming/structure nits.
This doesn't handle references into smart pointers (details below), but feel
free to do that in this patch, separately, or not at all.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:81
// This affects:
// - DependentTemplateSpecializationType,
// - DependentNameType
----------------
while here, I think UnresolvedUsingValueDecl and UnresolvedUsingTypenameDecl
can be added here :-)
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:217
+ if (E->isArrow()) {
+ if (!BaseType || !BaseType->isPointerType()) {
+ return;
----------------
If I'm understanding the AST correctly, this drops the `operator->` case, where
the base is a smart-pointer.
e.g. `void run(unique_ptr<Action<T>> X) { X->go(); }`
There are potentially two reasons then that `x->go` is a *dependent* memberexpr:
- `operator->` probably returns T*, but `unique_ptr` might be specialized (you
don't handle this)
- we probably want the primary `Action<T>::go`, but `Action` might be
specialized (you do handle this)
So it might be worth trying to get the pointee type (if the base type is a
TemplateSpecializationType, look up operator-> in the primary template, and
replace the base with its return type).
If you don't want to do this yet, we should leave a comment (like `FIXME:
handle unique_ptr<T> by looking up operator-> in the primary template`)
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:255
+ bool IsNonstaticMember) {
+ // This code was adapted in part from indexDependentReference() in
+ // IndexBody.cpp.
----------------
In the long term, I'm not sure this comment will be as useful as one saying
something like:
"Dependent name references like `vector<T>::size()` can't be definitively
resolved, but the using the definition from the primary template is probably
better than giving up"
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:278
+ });
+ for (const NamedDecl *D : Decls) {
+ Outer.add(D, Flags);
----------------
can we make this function return the decls, and make the callers call add() in
a loop?
That way this function could be reused for the `operator->` lookup, and could
be a standalone helper function outside this class. (Maybe eventually exposed
in AST.h, as I can imagine it being useful in other places, but static in this
file seems fine)
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:278
+ });
+ for (const NamedDecl *D : Decls) {
+ Outer.add(D, Flags);
----------------
sammccall wrote:
> can we make this function return the decls, and make the callers call add()
> in a loop?
>
> That way this function could be reused for the `operator->` lookup, and could
> be a standalone helper function outside this class. (Maybe eventually exposed
> in AST.h, as I can imagine it being useful in other places, but static in
> this file seems fine)
with multiple lookup results we could add all, discard all, or pick one
arbitrarily.
I'm not sure what the best policy is, but it's worth pointing out in a comment
what the cases are and what the choice is.
I think this should only affects overloads, where returning all seems
reasonable (resolution would be way too much work).
Can you add a test?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:470
+
+ R"cpp(// Heuristic resolution of method
+ template <typename T>
----------------
nit: dependent method (just so it's easier to search for)
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:501
+ }
)cpp"};
for (const char *Test : Tests) {
----------------
could you add a test for the `operator->` case too, even if we don't handle it
yet? (in which case with a FIXME)
Something like:
```
template <typename T> struct unique_ptr<T> { T* operator->(); };
template <typename T> struct S { void [[foo]](); }
template <typename T> void test(unique_ptr<S<T>>& V) { V->f^oo(); }
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71240/new/
https://reviews.llvm.org/D71240
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
