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