[PATCH] D127856: [clangd] Support multiline semantic tokens

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:948
+    } else {
+      // If a tokens length is past the end of the line, it should be treated as
+      // if the token ends at the end of the line and will not wrap onto the
----------------
sammccall wrote:
> sammccall wrote:
> > This wording is hard for me to follow. I think it's saying:
> > 
> > "If the token spans a line break, truncate it to avoid this"
> It seems it would be better to split into one token per line, rather than simply truncating.
> 
> Is truncation for simplicity or is there a reason to prefer it?
> 
> FWIW I think this wouldn't be too hard to implement if you reordered the tokenType/tokenModifiers above so this part is the last step, and just copied the whole SemanticToken object from the back of the Result. But on the other hand it's not terribly important, either.
> 
> At least I think we should have a comment for the truncate/split tradeoff.
> This wording is hard for me to follow. I think it's saying:

It is actually from LSP 😅 changed into a simpler version, I don't think we need references to LSP in here.

> Is truncation for simplicity or is there a reason to prefer it?

Yeah I didn't want to do a loop, as in theory there can be many lines not just two. Also we need to take care of the `Last` and delta calculations for the following tokens.
But it isn't as bad, i suppose. Changed into splitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127856