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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked 6 inline comments as done.
tom-anders added a comment.

> In D128826#3619956 <https://reviews.llvm.org/D128826#3619956>, @tom-anders wrote:
>
>>>> 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.
>
> Hmm, is the distinction that the type is written in the declaration (not `auto`)? I'm not sure it's worth specializing behavior for that scenario too much, when it's not too hard to go-to-definition on the specific token you want.
> The chance of guessing right has to be weighed against keeping the behavior predictable and comprehensible...

Yes that's what I meant, but I agree with you here.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1913
 // to target.
-static QualType unwrapFindType(QualType T) {
+static llvm::SmallVector<QualType> unwrapFindType(
+    QualType T, const HeuristicResolver* H) {
----------------
sammccall wrote:
> tom-anders wrote:
> > tom-anders wrote:
> > > sammccall wrote:
> > > > Ergonomically I might prefer `void unwrapFindType(QualType, ..., vector<QualType>& Out)` or so.
> > > > This tends to compose a bit better IMO by making all the cases look similar whether you're adding one type or several or combining lists.
> > > > 
> > > > You can still `return Out.push_back(T)` etc on a single line.
> > > Ah yes that makes sense. Didn't think about the `return Out.push_back(T)` trick to keep using early returns, thought I'd have to add a ton of `else if`s instead.
> > It makes the call side a bit uglier though - Is it okay to add a convenience wrapper `vector<QualType> unwrapFindType(QualType, HeuristicResolver*)` that forwards to the overload that uses the out-parameter?
> Just the final call, not the recursive calls, right? They can be `return unwrapFindType(...);` too.
> 
> It doesn't look ugly to me (2 extra lines at one callsite) but up to you.
Yeah just the final call. I just don't like adding (non-const) helper variables on the call site.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128826