[PATCH] D128826: Go-to-type on smart_ptr<Foo> now also shows Foo

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

> In D128826#3619339 <https://reviews.llvm.org/D128826#3619339>, @tom-anders wrote:
>
>> I'm using this with neovim's builtin LSP, where when there are multiple results it takes me to the first one, and also opens the quickfix window that shows all results. But I can see how the current implementation could be annoying with other editors.
>
> (Interesting, I'm using the same and find it annoying: it focuses the quickfix window and that's disruptive enough that I never noticed the cursor also moved. But https://github.com/neovim/neovim/pull/14878 lists some workarounds)

(Ah, I also forgot that I'm not actually using quickfix, but rather https://github.com/folke/trouble.nvim, which also gets focused but has a preview feature)

>> So here's what I thought: Say you have a variable that's a smart pointer and trigger textDocument/typeDefinition on it and there's only a single result that takes you straight to the type. In this case, you might not even know that the type is actually wrapped inside a smart pointer and might make wrong assumptions about the code. Of course you could've done a Hover beforehand, but in the case where there are 2 results, you don't need that extra step.
>
> Yeah. There's no guarantee the type in question is well-represented by a decl, but this is a bit confusing.
> I think it might still be worth it for specific types like `unique_ptr<Foo>`, but for `vector<Foo>` it's too surprising even if you want Foo 99% of the time.

Right, maybe it would make sense to go straight to `T` for declarations, but include `smart_ptr` when it's used in an expression? But that's something for a follow-up patch.

>> Maybe it would be best to make this configurable via the config file? Since most people are using "most editors", I think taking you straight to the type would be better, but I'm really not sure.
>
> Making it configurable doesn't actually solve the problem: we still need to think hard about what the best default is, because relatively few users ever configure anything.
> So let's not rule it out, but there's not much point doing it in this patch.
>
>> Btw, I think it would be useful to extend this feature to not only smart pointers, but also containers like std::vector. Then there'd also be the question what would happen for std::vector<std::unique_ptr<T>>: should there be 1, 2 or 3 results?
>
> Yeah I find this pretty compelling. I think we should start with the approach in this patch, returning {pointer, pointee}, and iterate from there (add containers, or templates in general, or omit the pointer type in certain special cases...).

Agreed. The heuristic there would be "template class with at least 1 type parameter, where all other parameters except the first have a default value", right? 
But maybe it even makes sense for stuff like `std::map`, so it's more like "template class, with `n>0` type template parameters, followed by `m>=0` defaulted template parameters"
Do we already have an issue for this? If not, I'd open a new one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128826