[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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
More information about the cfe-commits
mailing list