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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

(My first reaction was that this belongs in semanticHighlights() rather than toSemanticTokens, but I think you got it right - the single-line restriction is pretty unnatural from clang's point of view, and cuts across tokens generated in different places).



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


================
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:
> 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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:90
   uint32_t Modifiers = 0;
   Range R;
 
----------------
I think this deserves a comment like:

```
// This is clang's token bounds, which may span multiple lines.
// This may be split/truncated to a single line when required by LSP.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127856