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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits