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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
