hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:409
}},
+ {"semanticHighlighting",
+ llvm::json::Object{{"scopes", getTextMateScopeLookupTable()}}},
----------------
The proposal says `If the client declares its capabilities with respect to the
semantic highlighting feature, and if the server supports this feature too, the
server should set all the available TextMate scopes as a "lookup table" during
the initialize request.`.
now it seems that clangd always emit this lookup table to the client.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
+ write16be(static_cast<int>(Token.Kind), OS);
----------------
jvikstrom wrote:
> hokein wrote:
> > if the token is across multiple line, we will emit an ill-formed results.
> There's a FIXME above (which is where it should probably be handled). A bit
> unsure how to solve though. If a token is a block comment spanning multiple
> lines we would need to know the length of every line in the block comment.
> Probably something that can be solved with the ASTContext or SourceManager
> but that can't be accessed in this function.
oh, i missed that FIXME, the FIXME is a bit far away, maybe move it here (now
we assume the token is always on the same line).
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:74
+// FIXME: Factor this out into llvm/Support?
+std::string encodeBase64(const llvm::SmallVectorImpl<char> &U) {
+ static const char Table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
----------------
The name `U` doesn't provide much information, maybe `Bytes`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+ for (const auto &Line : TokenLines) {
+ llvm::SmallVector<char, 128> LineHighlights;
+ llvm::raw_svector_ostream OS(LineHighlights);
----------------
The code is a bit tricky here, if I understand the code correctly,
`LineHighlights` is the binary data of tokens (each `char` represents a byte).
Maybe `LineEncodingTokens`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:141
+ for (const auto &Token : Line.second) {
+ // First char is the start column on the line.
+ write32be(Token.R.start.character, OS);
----------------
don't repeat what the code does in the comment. Here we are encoding a `Token`
in a form specified in the proposal, I think we should have high-level comment.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:109
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector<SemanticHighlightingInformation> Info =
+ toSemanticHighlightingInformation(Tokens);
----------------
nit: ActualResults?
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:111
+ toSemanticHighlightingInformation(Tokens);
+ std::vector<SemanticHighlightingInformation> Correct = {
+ {1, "AAAAAQAEAAA="},
----------------
nit: ExpectedResults?
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:114
+ {3, "AAAACAAEAAAAAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
----------------
nit: we should use `EXPECT_*` when comparing the expected results and actual
results.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
