[PATCH] D108320: Add semantic token modifier for non-const reference parameter

[Nathan Ridge via Phabricator via cfe-commits]

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