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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
