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

ckandeler added a comment. In D136594#3991530 , @hokein wrote: > This patch causes a clangd crash on dependent code (e.g. running on > `ASTMatchers.h`), fixed in > https://github.com/llvm/llvm-project/commit/bcb457c68e20120f0bdd0a59e4b4ce90b8121310.

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

hokein added a comment. This patch causes a clangd crash on dependent code (e.g. running on `ASTMatchers.h`), fixed in https://github.com/llvm/llvm-project/commit/bcb457c68e20120f0bdd0a59e4b4ce90b8121310. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

nridge added a comment. Thanks, Christian, for your work on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 ___ cfe-commits mailing list

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG647d314eb059: [clangd] Add support for semantic token type operator (authored by ckandeler). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/

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

ckandeler updated this revision to Diff 482103. ckandeler added a comment. Renamed modifier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp

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

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the explanation! It makes sense. And given this motivation I don't think inverting to "builtin" is a good idea. The impact is a bit limited because it's not a standard

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

ckandeler added a comment. In D136594#3987852 , @nridge wrote: > - Does the implementation handle explicit operator calls, e.g. > `a.operator+(b)`? If so, it would be nice to add some test coverage. It does now. Repository: rG LLVM Github Monorepo

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

ckandeler updated this revision to Diff 482055. ckandeler added a comment. Added more test cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 Files:

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

nridge added a comment. While we're waiting for Sam to weigh in on the modifier question, a couple of comments about the details of the patch: - Does the implementation handle explicit operator calls, e.g. `a.operator+(b)`? If so, it would be nice to add some test coverage. - I see the patch

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

nridge added a comment. In D136594#3973908 , @ckandeler wrote: > In D136594#3973812 , @sammccall > wrote: > >> For my part, I still need to understand why we want the >> `builtin`/`UserModified` modifier. (The

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

ckandeler added a comment. In D136594#3973812 , @sammccall wrote: > For my part, I still need to understand why we want the > `builtin`/`UserModified` modifier. (The `operator` highlight kind seems > obvious to me). We make this distinction in our

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

sammccall added a comment. For my part, I still need to understand why we want the `builtin`/`UserModified` modifier. (The `operator` highlight kind seems obvious to me). From earlier comments: >> Can you give some background here or on the bug tracker about what kind of >> distinction

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

ckandeler added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ckandeler added a comment. Now seems to do what we want. Should I switch the modifier around? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 ___ cfe-commits

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

ckandeler updated this revision to Diff 478146. ckandeler added a comment. Using operators on expressions with dependent types now considered "complex", Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594

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

ckandeler added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, sammccall wrote: > ckandeler wrote: > > sammccall wrote: > > > sammccall wrote: > > > > nridge wrote: > > > > >

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, ckandeler wrote: > sammccall wrote: > > sammccall wrote: > > > nridge wrote: > > > > ckandeler wrote: > > > > >

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

ckandeler added a comment. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, sammccall wrote: > sammccall wrote: > > nridge wrote: > > > ckandeler wrote: > > > > ckandeler wrote: > > > > > sammccall

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

ckandeler updated this revision to Diff 477919. ckandeler added a comment. Added template example. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 Files:

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

sammccall added a comment. Regarding AugmentsSyntaxTokens: - in general this seems like a nice thing to support, though I'm not aware of "big" `!AugmentsSyntaxTokens` clients so it's not urgent - I think the right design for that is to (conditionally) run the lexer and mark only tokens that

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

ckandeler added a comment. In D136594#3950334 , @nridge wrote: > The current patch will produce an operator token in the operator case but not > the declarator case, thereby achieving an effect that client-side > highlighting couldn't. As such, I guess

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

nridge added a comment. (I envisioned the implementation of `augmentsSyntaxTokens=false` to be "loop over the lexer tokens and turn a subset of them into additional tokens to return over LSP". The fact that that would have meant producing an "operator" token even for `tok::star` which is

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

nridge added a comment. In D136594#3940482 , @sammccall wrote: > An (IMO) useful distinction that can't be found by the lexer is the use of > `*` as a declarator (`int *x`) vs an operator (`return *x`). I failed to appreciate the implication of this

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

ckandeler added a comment. > Should I perhaps add the augmentsSyntaxTokens option and rebase this patch to > make it its first user? So I went ahead and did this. Opinions? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/

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

ckandeler updated this revision to Diff 477178. ckandeler added a comment. Make highlighting of built-in operators conditional Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 Files:

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

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

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

nridge added a comment. In D136594#3940482 , @sammccall wrote: > I think since LSP specifies an `operator` SemanticTokenType, we should use it > unless there's a *really* strong reason not to. My bad, I completely overlooked that LSP has a standard

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

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

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

sammccall added a comment. In D136594#3940143 , @nridge wrote: > I believe highlighting of **built-in** operators should be out of scope for > semantic highlighting, at least in the default mode; client-side highlighting > should be sufficient for

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

ckandeler added a comment. In D136594#3940143 , @nridge wrote: > A couple of high-level thoughts on this: > > 1. Based on the discussion in https://github.com/clangd/clangd/issues/1115, I > believe highlighting of **built-in** operators should be out of

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

nridge added a comment. A couple of high-level thoughts on this: 1. Based on the discussion in https://github.com/clangd/clangd/issues/1115, I believe highlighting of **built-in** operators should be out of scope for semantic highlighting, at least in the default mode; client-side highlighting

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

nridge added a comment. For reference, earlier work along these lines which never landed: https://reviews.llvm.org/D119077 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594

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

ckandeler created this revision. ckandeler added a reviewer: sammccall. Herald added subscribers: kadircet, arphaman. Herald added a project: All. ckandeler requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra.