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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 21 06:28:09 PDT 2022

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(
please have the first token start somewhere other than column 0

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list