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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
