[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 3 03:46:05 PDT 2019


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





More information about the cfe-commits mailing list