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