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
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 

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to