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