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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

a few more comments from my side.



================
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;
----------------
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. 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:70
+// Return a line-by-line diff between two highlightings.
+//  - if the tokens on a line are the same in both hightlightings this line is
+//  omitted.
----------------
nit:  add `, ` before `this`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:73
+//  - if a line exists in New but not in Old the tokens
+//  on this line is emitted., we emit the tokens on this line
+//  - if a line not exists in New but in Old an empty
----------------
could you refine this sentence, I can't parse it.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+//  on this line is emitted., we emit the tokens on this line
+//  - if a line not exists in New but in Old an empty
+//  line is emitted (to tell client to clear the previous highlightings on this
----------------
and here as well.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef<HighlightingToken> New,
+                  ArrayRef<HighlightingToken> Old, int NLines);
 
----------------
ilya-biryukov wrote:
> Could you document what `NLines` is? Specifically, whether it's the number of lines for `New` or `Old`.
> 
could we use a more describe name for  `NLines`? maybe `MaxLines`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475