[PATCH] D134728: [clangd] Add highlighting modifiers "constructor" and "destructor"

Thu Sep 29 00:46:27 PDT 2022

sammccall added a comment.

TL;DR: let's go with this patch as-is rather than borrowing problems from the future. This enum isn't growing very rapidly.

In D134728#3822775 <https://reviews.llvm.org/D134728#3822775>, @ckandeler wrote:

> In D134728#3822653 <https://reviews.llvm.org/D134728#3822653>, @sammccall wrote:
>
>> It doesn't scale very well though: we're limited to 30 modifiers in total, this patch brings us up to 16, if we followed this class.constructor precedent for function.operator, class.constructor.copy enum.scoped etc we could end up exhausting this.

BTW, I remembered the other idea that could eat a bunch of the modifiers: rainbow highlighting a la https://github.com/clangd/clangd/issues/889. The idea is you could encode e.g. an 8 bit hash of the symbol identity as the presence/absence of 8 modifiers. But we never shipped it, and I suspect 8 would be enough.

> In light of this (and assuming it really has to be this way)

FWIW the reason is that modifiers are encoded as an integer, and the spec says integers are <2^31-1 for interop purposes. (I'm not sure why they didn't go for 2^53-1, everyone has either doubles or 64-bit integers right? sigh).

> would it make sense to introduce a generic modifier that changes its meaning depending on the main highlighting kind? It would have the drawback of not being self-documenting

Neat idea. That's workable at a technical level, but requires editors/themes to be pretty tightly coupled to this idiosyncracy of clangd. So I'm not sure it's a good fit for the loosely-coupled LSP ecosystem.

> but with the given constraints might just be a pragmatic solution

Yeah, however the constraints aren't very tight at this point, I was just thinking out loud a bit. Let's go for the simple thing for now - we may never need something cleverer, and if we do then we'll have more examples to drive the design. I think that's worth the potential breaking change.

>   // Class -> Constructor/Destructor

We'd need Generic1 and Generic2 so you can distinguish constructor/destructor right?
If the distinction isn't important, maybe we should add a single "constructor or destructor" modifier...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134728