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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly final NITs from me, but there is an important one about removing the `erase` call on `didOpen`.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467
+    std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+    FileToHighlightings.erase(File);
+  }
----------------
Now that the patch landed, this is obsolete. Could you remove?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+    FileID MainFileID = SM.getMainFileID();
+    unsigned int FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+    int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);
----------------
NIT: use `unsigned` instead of `unsigned int`


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+  virtual void onHighlightingsReady(
+      PathRef File, std::vector<HighlightingToken> Highlightings, int NLines) {}
 };
----------------
NIT: could you document `NLines`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef<HighlightingToken> NewLine(New.begin(),
+                                      /*length*/0UL);
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:310
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits<int>::max() && Line < NLines;
+       Line = NextLine()) {
----------------
`Line != intmax` is redundant (NLines is <= intmax by definition).
Maybe remove it altogether to simplify the loop condition?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef<HighlightingToken> New,
+                  ArrayRef<HighlightingToken> Old, int NLines);
 
----------------
Could you document what `NLines` is? Specifically, whether it's the number of lines for `New` or `Old`.



================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:60
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
----------------
NIT: add documentation on how this should be used
Most importantly, the fact that we need to put `^` on all changed lines should be mentioned.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:66
+
+  std::map<int, std::vector<HighlightingToken>> ExpectedLines;
+  for (const Position &Point : NewTest.points()) {
----------------
NIT: use `llvm::DenseMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475