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