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

Tue Jul 30 02:34:28 PDT 2019

jvikstrom added inline comments.

================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef<HighlightingToken> NewLine(New.begin(),
+                                      /*length*/0UL);
----------------
ilya-biryukov wrote:
> Could we try to find a better name for this? Having `Line` and `NextLine()` which represent line numbers and `NewLine` which represents highlightings, creates some confusion.
I renamed the `Line` and `NextLine()` instead. Could also rename `NewLine` to something like `NewLineHighlightings` but I feel like it almost becomes to verbose at that point.

================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:309
+  // If the New file has fewer lines than the Old file we don't want to send
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits<int>::max() && Line < NLines;
----------------
hokein wrote:
> nit: I believe theia is crashing because of this? could we file a bug to theia? I think it would be nice for client to be robust on this case. 
I seem to be misremembering, when I first started testing in theia I think I was crashing the client (due to a broken base64 encoding function, will take a look and see what was actually happening, can't quite remember)
It actually seems like theia just ignores any out of bounds highlightings.

So this could be removed, it will just sometimes return highlightings that are outside of the file and hopefully any future clients would handle that as well. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475