[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

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