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

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
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 ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136594