[PATCH] D136594: [clangd] Add support for semantic token type "operator"

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
ckandeler wrote:
> ckandeler wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
> > > > > (Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)
> > > > as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5
> > > > 
> > > > If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.
> > > Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> > > (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)
> > > as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5
> > > 
> > > If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.
> > 
> > I use "userProvided" here in the sense of "not built-in" or "overloaded". I do not cling to the term, and have also suggested the opposite default of using "builtin" in another comment.
> > Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> 
> True, I have not considered this.
> 
> > (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)
> 
> Intuitively, it seems we should be conservative and not claim the operator is overloaded unless we know it is. So "built-in" might then mean "we can't prove it's not a built-in". It's probably not worth to introduce a modifier CouldBeEitherWay just to explicitly express ambiguity ;)
> Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)

In my mind, "go-to-definition on this operator symbol will take me to a function declaration/definition" is a good match for "I want this colored differently". (Which would imply treating dependent operator calls where we can't figure out an overloaded operator target even heuristically, as "built-in".)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136594