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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the code looks clearer now!



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:609
     FixItsMap.erase(File);
+    std::lock_guard<std::mutex> HLock(HighlightingsMutex);
+    FileToPrevHighlightings.erase(File);
----------------
nit: I would create a new `{}` block merely for the hightlightings.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1110
     PathRef File, std::vector<HighlightingToken> Highlightings) {
+  llvm::ArrayRef<HighlightingToken> Prev;
+  {
----------------
this seems unsafe, we get a reference of the map value, we might access it without the mutex being guarded. 

```
std::vector<HighlightingToken> Old;
{
    std::lock_guard<std::mutex> Lock(HighlightingsMutex);
    Old = std::move(FileToHighlightings[File]);
}
```


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1124
+    std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+    FileToPrevHighlightings[File] = Highlightings;
+  }
----------------
`FileToPrevHighlightings[File] = std::move(Highlightings);`


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:140
+  std::mutex HighlightingsMutex;
+  llvm::StringMap<std::vector<HighlightingToken>> FileToPrevHighlightings;
 
----------------
nit: I'd drop `Prev`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:229
+// Get the highlights for \c Line where the first entry for \c Line is
+// immediately preceded by \c OldLine
+ArrayRef<HighlightingToken> takeLine(ArrayRef<HighlightingToken> AllTokens,
----------------
nit: I'm not sure I understand the comment.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:233
+                                     int Line) {
+  return ArrayRef<HighlightingToken>(OldLine.end(), AllTokens.end())
+      .take_while([Line](const HighlightingToken &Token) {
----------------
nit: this implies that OldLine and AllTokens must ref to the same container, could you refine the `OldLine` as a start `Iterator`? 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef<HighlightingToken> NewLine(Highlightings.data(),
+                                      Highlightings.data()),
----------------
IIUC, we are initializing an empty ArrayRef, if so, how about using `NewLine(Highlightings.data(), /*length*/0)`? `NewLine(Highlightings.data(), Highlightings.data())` is a bit weird, it took me a while to understand the purpose.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:257
+                                      Highlightings.data()),
+      OldLine = {PrevHighlightings.data(), PrevHighlightings.data()};
+  // ArrayRefs to the new and old highlighting vectors.
----------------
nit: split it into a new statement.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:259
+  // ArrayRefs to the new and old highlighting vectors.
+  ArrayRef<HighlightingToken> Current = {Highlightings},
+                              Prev = {PrevHighlightings};
----------------
we don't change Current and Prev below, how about re-using `Highlightings` and `PrevHighlightings`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:71
+/// in \c Highlightings an empty line is added. Returns the resulting
+/// HighlightingTokens grouped by their line number. Assumes the highlightings
+/// are sorted by the tokens' ranges.
----------------
I believe `Assumes the highlightings are sorted by the tokens' ranges.` is a requirement for this function?

If so, mark it more explicitly in the comment, like 

```
///
/// REQUIRED: OldHighlightings and NewHighlightings are sorted.
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+std::vector<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> Highlightings,
+                  ArrayRef<HighlightingToken> PrevHighlightings);
----------------
nit: I'd use name the parameters symmetrically, `NewHighlightings` and `OldHighlightings`.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:32
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple<ParsedAST, std::vector<HighlightingToken>,
+           std::vector<HighlightingToken>>
----------------
we don't need `ParsedAST`  now I think.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:265
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector<
+      std::pair<std::vector<int>, std::pair<const char *, const char *>>>
----------------
this is a complicated structure, could you add some comments describing the test strategy here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475