[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

[Nathan Ridge via Phabricator via cfe-commits]

nridge planned changes to this revision.
nridge marked an inline comment as done.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:152
+          // Don't bother computing the offset for the end of the line, just use
+          // zero. The client will treat this highlighting kind specially, and
+          // highlight the entire line visually (i.e. not just to where the text
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > nridge wrote:
> > > > hokein wrote:
> > > > > This seems too couple with VSCode client, I would prefer to calculate the range of the line and return to the client.
> > > > > 
> > > > > Is there any big differences in VSCode between highlighting with the `isWholeLine` and highlighting with the range of the line? 
> > > > I took some screenshots to illustrate to difference.
> > > > 
> > > > Highlighting only to the end of the line of text:
> > > > 
> > > > {F10158508}
> > > > 
> > > > Highlighting the whole line:
> > > > 
> > > > {F10158515}
> > > > 
> > > > I think the first one looks pretty bad, and is inconsistent with existing practice.
> > > > 
> > > > Note also that the suggestion is not to special-case the VSCode client specifically; it's to special-case one particular highlighting, which any client can implement.
> > > > 
> > > > If this special-casing is really unpalatable, we could instead try this suggestion by @sammccall:
> > > > 
> > > > > Failing that, I'd suggest encoding a list of line-styles on SemanticHighlightingInformation, that should be combined with any tokens on that line.
> > > > 
> > > > I guess one consideration when evaluating these options is, do we expect to use that "list of line-styles" for anything else in the future? I can't think of anything at the moment, but perhaps there are other uses for it.
> > > > 
> > > > If not, we could do something slightly simpler, and add a single `isInactive` flag to `SemanticHighlightingInformation`.
> > > Three approaches seem feasible here:
> > > 1. clients that know about the specific scope can extend it to the whole line. 
> > > 2. [0,0) or so indicates "highlight the whole line"
> > > 3. use a dedicated property for line styles (vs token styles)
> > > 
> > > 3 is clearly better than 2 I think, it's more explicit. I don't have a strong opinion of 1 vs 3, but if going with 1 I think it's a good idea to measure the line as Haojian says, so we at least get a basic version of the feature if the client doesn't know about line styles.
> > > 
> > > > I guess one consideration when evaluating these options is, do we expect to use that "list of line-styles" for anything else in the future? I can't think of anything at the moment
> > > Preprocessor directives maybe? (Though these are easy enough for clients to highlight with regex)
> > I can't say whether highlighting the line is better than highlighting the range of the line text, but below is the how the inactive TS code is highlighted in VSCode (only the range of text), I personally prefer this style.
> > 
> > {F10189885}
> I think that's an argument for making sure clients clearly distinguish between regular tokens and marking lines: overlapping tokens don't compose well, but we can easily say lines and token styles should compose.
> 
> (That particular style is not for me, but it doesn't matter)
I find this a convincing argument for using line styles, thanks.

Especially since, at some point in the future, I would like us to be able to produce regular token highlightings for inactive code, much like in that TS code screenshot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67536/new/

https://reviews.llvm.org/D67536