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