ilya-biryukov added a comment.
Mostly final NITs from me, but there is an important one about removing the
`erase` call on `didOpen`.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467
+ std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+ FileToHighlightings.erase(File);
+ }
----------------
Now that the patch landed, this is obsolete. Could you remove?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+ FileID MainFileID = SM.getMainFileID();
+ unsigned int FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+ int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);
----------------
NIT: use `unsigned` instead of `unsigned int`
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+ virtual void onHighlightingsReady(
+ PathRef File, std::vector<HighlightingToken> Highlightings, int NLines)
{}
};
----------------
NIT: could you document `NLines`
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+ // ArrayRefs to the current line in the highlights.
+ ArrayRef<HighlightingToken> NewLine(New.begin(),
+ /*length*/0UL);
----------------
Could we try to find a better name for this? Having `Line` and `NextLine()`
which represent line numbers and `NewLine` which represents highlightings,
creates some confusion.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:310
+ // highlightings beyond the end of the file. That might crash the client.
+ for (int Line = 0; Line != std::numeric_limits<int>::max() && Line < NLines;
+ Line = NextLine()) {
----------------
`Line != intmax` is redundant (NLines is <= intmax by definition).
Maybe remove it altogether to simplify the loop condition?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef<HighlightingToken> New,
+ ArrayRef<HighlightingToken> Old, int NLines);
----------------
Could you document what `NLines` is? Specifically, whether it's the number of
lines for `New` or `Old`.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:60
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+ Annotations OldTest(OldCode);
----------------
NIT: add documentation on how this should be used
Most importantly, the fact that we need to put `^` on all changed lines should
be mentioned.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:66
+
+ std::map<int, std::vector<HighlightingToken>> ExpectedLines;
+ for (const Position &Point : NewTest.points()) {
----------------
NIT: use `llvm::DenseMap`
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
