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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:140
     }
+    // Add inactive highlighting tokens.
+    const SourceManager &SM = AST.getSourceManager();
----------------
I think this comment could be clearer, e.g. // Add tokens indicating lines skipped by the preprocessor.


================
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
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536