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

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added a comment.

In D67536#1691107 <https://reviews.llvm.org/D67536#1691107>, @hokein wrote:

> Could you add tests for this?


Certainly, I just wanted to discuss the general approach first, as it will affect what the tests look like.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536