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