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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:172
+                              const HighlightingToken &Rhs) {
+  return Lhs.R.start.line == Rhs.R.start.line
+             ? Lhs.R.start.character < Rhs.R.start.character
----------------
I think this is just Lhs.R.start < Lhs.R.end

(if it's not, we should add the `operator<`)

is it enforced somewhere that you don't have two highlights at the same spot?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:190
+  // highlightings for old ASTs)
+  std::lock_guard<std::mutex> Lock(PrevMutex);
+  // The files are different therefore all highlights differ.
----------------
holding the lock while doing all the diffing seems dubious

You could reasonably put the cache as a map<file, highlights> in ClangdServer, then you don't have to deal with not having the cache for the current file.

You'd need to lock the map itself, but note that no races on updating individual entries are possible because onMainAST is called synchronously from the (per-file) worker thread.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+  // PrevHighlightings and Highlightings.
+  int I = 0, PI = 0, EndI = Highlightings.size(),
+      EndP = PrevHighlightings.size();
----------------
Whatever you do about storage, please pull out the diff(old, new) function so you can unit test it.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+  // PrevHighlightings and Highlightings.
+  int I = 0, PI = 0, EndI = Highlightings.size(),
+      EndP = PrevHighlightings.size();
----------------
sammccall wrote:
> Whatever you do about storage, please pull out the diff(old, new) function so you can unit test it.
(llvm uses `unsigned` for indices. It's a terrible idea, but it's used fairly consistently...)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:209
+      EndP = PrevHighlightings.size();
+  while (I < EndI && PI < EndP) {
+    const HighlightingToken &Current = Highlightings[I];
----------------
I can believe this is correct, but it's hard to verify as it's a bit monolithic and... index-arithmetic-y.

could you factor out something like:

```
using TokRange = ArrayRef<HighlightingToken>;

// The highlights for the current line.
TokRange OldLine = {PrevHighlightings.data(), 0}, NewLine = {Highlightings.data(), 0};
// iterate over lines until we run out of data
for (unsigned Line = 0; OldLine.end() < PrevHighlightings.end() || NewLine.end() < PrevHighlightings.end(); ++Line) {
  // Get the current line highlights
  OldLine = getLineRange(PrevHighlightings, Line, OldLine);
  NewLine = getLineRange(Highlightings, Line, NewLine);
  if (OldLine != NewLine)
    emitLine(NewLine, Line);
}

// get the highlights for Line, which follows PrevLine
TokRange getLineRange(TokRange AllTokens, unsigned Line, TokRange PrevLine) {
  return makeArrayRef(PrevLine.end(), AllTokens.end()).take_while( Tok => Tok.R.start.line == Line);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64475/new/

https://reviews.llvm.org/D64475