nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:314
// (these tend to be vague, like Type or Unknown)
+// - Resolved tokens (i.e. without the "dependent-name" modifier) with kind
+// "Unknown" are less reliable than resolved tokens with other kinds
----------------
sammccall wrote:
> nridge wrote:
> > We should consider the case where a dependent name is passed by non-const
> > reference, for example:
> >
> > ```
> > void increment_counter(int&);
> >
> > template <typename T>
> > void bar() {
> > increment_counter(T::static_counter);
> > }
> > ```
> >
> > This case does not work yet with the current patch (the dependent name is a
> > `DependentScopeDeclRefExpr` rather than a `DeclRefExpr`), but we'll want to
> > make it work in the future.
> >
> > With the conflict resolution logic in this patch, the `Unknown` token kind
> > from `highlightPassedByNonConstReference()` will be chosen over the
> > dependent token kind.
> >
> > As it happens, the dependent token kind for expressions is also `Unknown`
> > so it doesn't matter, but perhaps we shouldn't be relying on this. Perhaps
> > the following would make more sense:
> >
> > 1. a token with `Unknown` as the kind has the lowest priority
> > 2. then a token with the `DependentName` modifier (next lowest)
> > 3. then everything else?
> The conflict-resolution idea is subtle (and IME hard to debug). I'm wary of
> overloading it by deliberately introducing "conflicts" that should actually
> be merged. Did you consider the idea of tracking extra modifiers separately
> and merging them in at the end?
>
> ---
>
> BTW: we're stretching the meaning of `Unknown` here. There are two subtly
> different concepts:
> - clangd happens not to have determined the kind of this token, e.g. because
> we missed a case (uses in this patch)
> - clangd has determined that per C++ rules the kind of token is ambiguous
> (uses prior to this patch)
> Call me weird, but I have "Unknown" highlighted in bright orange in my
> editor, because I want to know about the second case :-)
I don't have a strong opinion on the options here, just wanted to chime in and
say I also highlight `Unknown` prominently for similar reasons :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108320/new/
https://reviews.llvm.org/D108320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
