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