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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 16 01:55:08 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list