[PATCH] D95701: [clangd] Add semanticTokens modifiers for function/class/file/global scope

[Nathan Ridge via Phabricator via cfe-commits]

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

Looks good to me!



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:379
+  // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
+  // Avoid caching linkage if it may change after enclosing code completion.
+  if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
----------------
code completion? I'm a bit lost :)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:466
     case TemplateArgument::TemplateExpansion:
+      // FIXME: I don't understand why this is DependentType.
       H.addToken(L.getTemplateNameLoc(), HighlightingKind::DependentType);
----------------
The testcase which relies on this is this one:

```
      // Dependent template name
      R"cpp(
      template <template <typename> class> struct $Class[[A]] {};
      template <typename $TemplateParameter[[T]]>
      using $Typedef[[W]] = $Class[[A]]<
        $TemplateParameter[[T]]::template $DependentType[[Waldo]]
      >;
```

However, it does appear that we get into here even for non-dependent template template arguments (but then also get a non-dependent highlighting kind via `findExplicitReferences()`, and end up discarding the `DependentType` via `resolveConflict()`).


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:785
+  case HighlightingModifier::FunctionScope:
+    return "functionScope";
+  case HighlightingModifier::ClassScope:
----------------
Maybe add a `// nonstandard` comment for these as well


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:82
+  FileScope,
+  GlobalScope,
+
----------------
Would it make sense to call this `NamespaceScope` instead?

I understand it's "global" in the sense that it's visible to other translation units, but it's not "global" in the sense that it's not necessarily in the global namespace.

(On the other hand, I can see how `FileScope` symbols are also "namespace scope", so... could go either way.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95701