[PATCH] D44247: [clangd] Use identifier range as the definition range.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 8 08:21:41 PST 2018
sammccall added a comment.
In https://reviews.llvm.org/D44247#1031366, @sammccall wrote:
> In https://reviews.llvm.org/D44247#1031345, @malaperle wrote:
> > I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894
> Interesting - that seemed like a more natural interpretation of LSP to me.
> Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)
Man, this seemed compelling to me, but there's a little bit of wiggle room in the spec (what's the "definition location"), so we looked at the MS language servers...
... and both their TS and C++ implementations return the range of the name only, despite that (IMO) being a weird interpretation of the spec.
As for VSCode:
- the "ctrl-to-hover" behavior starts at beginning of the line containing the range, and has a heuristic for when to stop (even if you return the whole definition range, I think). So whole-range is a bit better here (particularly when type/template is on a separate line) but actually still not great.
- "peek definition" shows the selected range in the middle of a block, with the range highlighted. Having the whole code highlighted actually looks kinda bad :/
- "go to definition" puts your cursor at the start of the range, and the identifier seems much better here.
So I *want* to agree, but we'll be fighting the other language servers and editors (and @ioeric, @ilya-biryukov who think we'd be breaking more important workflows)... I think we're actually better off just returning the name.
rCTE Clang Tools Extra
More information about the cfe-commits