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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

based on the offline discussion, now I understand the patch better (thanks).

some comments on the interface.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60
+      PathRef File,
+      std::vector<std::pair<int, std::vector<HighlightingToken>>> Highlightings)
+      override;
----------------
how about creating a new structure `LineHighlightingTokens` to represent the line-based tokens?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:82
   bool SemanticHighlighting;
+  HighlightingDiffer Differ;
 };
----------------
If we make it as a pointer, the `bool SemanticHighlighting` is not needed (just follow the what the other field `FIndex` does).


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  const FileEntry *CurrentFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  std::string CurrentPath = CurrentFileEntry->tryGetRealPathName().str() +
+                            CurrentFileEntry->getName().str();
----------------
we could get the canonical path from `onMainAST` callback (the first parameter `PathRef Path`).


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:60
+class HighlightingDiffer {
+  std::map<std::string, std::vector<HighlightingToken>> PrevHighlightingsMap;
+  std::mutex PrevMutex;
----------------
nit: llvm::StringMap.

I think this map is storing all file highlightings, maybe call it `FileToHighlightings`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+  std::vector<std::pair<int, std::vector<HighlightingToken>>>
+  diffHighlightings(const ParsedAST &AST,
+                    std::vector<HighlightingToken> Highlightings);
----------------
this method does two things, 
1) calculate the diff between old and new
2) replace the old highlighting with new highlightings

but the name just reflects 1).

I think we just narrow it to 2) only:

```
// calculate the new highlightings, and return the old one.
std::vector<HighlightingToken> updateFile(PathRef Path, std::vector<HighlightingToken> NewHighlightings);

// to get diff, in `onMainAST`
diffHighlightings(differ.updateFile(Path, NewHighlightings), NewHighlightings);
```



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:82
+  std::vector<std::pair<int, std::vector<HighlightingToken>>>
+  diffHighlightings(ArrayRef<HighlightingToken> Highlightings,
+                    ArrayRef<HighlightingToken> PrevHighlightings);
----------------
this utility method doesn't use any internal member of the class, we could pull it out or make it static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475