[PATCH] D44247: [clangd] Use identifier range as the definition range.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44247#1031429, @sammccall wrote:

> 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.


Good points, thanks for investigating this. I agree that it should return the range of the name only so the direction of this patch looks good to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44247