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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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)
----------------
nridge wrote:
> code completion? I'm a bit lost :)
Whoops, fixed.

(This was copied from a utility function that's used in code completion ranking - I planned to share it, but it turns out we want slightly different semantics. Code completion happens inside callbacks that happen mid-parse, and if you call getLinkageInternal() at just the wrong time, you end up caching an intermediate state and crash. But I removed the check as it's not relevant here)


================
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);
----------------
nridge wrote:
> nridge wrote:
> > 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()`).
> I see you've already figured this out in D95706 :)
Oops, yes. I've adjusted the comment here so the intermediate state makes sense :-)
(I'd rather not emit dubious tokens and then rely on conflicts to discard them, it seems hard to maintain)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:82
+  FileScope,
+  GlobalScope,
+
----------------
nridge wrote:
> 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.)
Yeah, the file-scope thing. (Maybe it's not an important distinction, but it seems pretty interesting for functions at least)

The other consideration is that I harbor a little bit of hope we could get some convergence across implementations, so avoiding lang-specific terms is nice. In any case, client implementers are probably not C++ people!
(of course we can use different names internally than we use in the protocol, but it seems less confusing to align them)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95701