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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.


================
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);
----------------
hokein wrote:
> 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`?
They aren't "fully" encoded yet though, they get encoded after the inner loop is done. How about `LineByteTokens`?



================
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);
----------------
hokein wrote:
> 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).
I think that this should probably be handled above (so if a token covers `N` different lines it would be separated into `N` tokens in the `TokenLines` map. 
I don't really see how it would be possible to handle that in this loop (without basically putting all the smallvectors for every line in another vector and  writing to `Lines` in another loop after the loop is done. But it feels like that would be way more complex) 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:8
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHTING_H
----------------
hokein wrote:
> The semantic highlighting is not in LSP yet, I think we need some documentations here to explain some more details about this feature in clangd, like the implementations are based on the proposal (https://github.com/microsoft/vscode-languageserver-node/pull/367).  
Changed the initialize-params test to have semantic highlighting enabled as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63919/new/

https://reviews.llvm.org/D63919