ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:274
+std::vector<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings,
+ ArrayRef<HighlightingToken> OldHighlightings) {
----------------
NIT: maybe rename to `New` and `Old`, the suffix of the name could be easily
inferred from the variable type (clangd has hover/go-to-definition to find the
type quickly, the code is rather small so its should always be visible on the
screen too).
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:276
+ ArrayRef<HighlightingToken> OldHighlightings) {
+ // FIXME: There's an edge case when tokens span multiple lines. If the first
+ // token on the line started on a line above the current one and the rest of
----------------
ilya-biryukov wrote:
> Do you have any ideas on how we can fix this?
>
> I would simply split those tokens into multiple tokens of the same kind at
> newlines boundaries, but maybe there are other ways to handle this.
>
> In any case, could you put a suggested way to fix this (in case someone will
> want to pick it up, they'll have a starting point)
NIT: add assertions that highlightings are sorted.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:286
+ // incorrectly removed.
+ std::vector<LineHighlightings> LineTokens;
+ // ArrayRefs to the current line in the highlights.
----------------
NIT: maybe mention `diff` in the name somehow. I was confused at first, as I
thought it's all highlightings grouped by line, not the diff.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:292
+ /*length*/0UL);
+ auto NewEnd = NewHighlightings.end();
+ auto OldEnd = OldHighlightings.end();
----------------
NIT: maybe just inline `NewEnd` and `OldEnd` to save a few lines?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+ auto OldEnd = OldHighlightings.end();
+ for (int Line = 0; NewLine.end() < NewEnd || OldLine.end() < OldEnd; ++Line)
{
+ NewLine = takeLine(NewHighlightings, NewLine.end(), Line);
----------------
Could we skip directly to the next interesting line?
The simplest way to do this I could come up with is this:
```
auto NextLine = [&]() {
int NextNew = NewLine.end() != NewHighlightings.end() ?
NewLine.end()->start.line : numeric_limits<int>::max();
int NextOld = ...;
return std::min(NextNew, NextOld);
}
for (int Line = 0; ...; Line = NextLine()) {
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
