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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D67536#1697533 <https://reviews.llvm.org/D67536#1697533>, @nridge wrote:

> How would one even measure the line length? `SourceManager` doesn't sem to have a method like `getLineLength()` or similar.


Yes, there is no existing API for that, I think you'd need to get the source code from the SM at the specific offset, and do the `\n` counting.



================
Comment at: clang-tools-extra/clangd/ParsedAST.h:100
 
+  const std::vector<SourceRange> &getSkippedRanges() const {
+    return SkippedRanges;
----------------
Instead of adding new member and methods in `ParsedAST`, I think we can do it in `CollectMainFileMacros` (adding a new field SkippRanges in `MainFileMacros`), then we can get the skipped ranges for preamble region within the main file for free.


================
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:
> 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}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536