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

Reply via email to