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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:947
+    Out->tokenModifiers = Tok.Modifiers;
+    Last = Tok;
 
----------------
this copy feels gratuitous, can we just use Tokens.back(), or do the copy in the rare case?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:954
+      // line.
+      // This is slow, but code rarely has multiline tokens.
+      // FIXME: There's a client capability for supporting multiline tokens,
----------------
This isn't literally true - files with multiline raw strings aren't rare. maybe "multiline tokens are rare"?
(even so I wonder if there's a real performance problem here...)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:960
+      // length.
+      for (int I = Tok.R.start.line; I != Tok.R.end.line; ++I) {
+        auto LineEnd = Code.find('\n', TokStartOffset);
----------------
can we use < rather than != here? (or an assert)
I realize something has to have gone horribly wrong to infloop here, but it's something nonlocal...


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:971
+        Out->deltaStart = 0;
+        Out->tokenType = static_cast<unsigned>(Tok.Kind);
+        Out->tokenModifiers = Tok.Modifiers;
----------------
reassigning fields one by one seems fragile, copy the last token and then overwrite the fields needed?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:1007
+  llvm::StringRef AnnotatedCode = R"cpp(
+[[fo
+o
----------------
please have the first token start somewhere other than column 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127856