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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
