[PATCH] D120398: [format] follow up: Use unsigned char as the base of all enums in FormatStyle

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.
Herald added a project: All.

In D120398#3352024 <https://reviews.llvm.org/D120398#3352024>, @Quuxplusone wrote:

> In D120398#3351998 <https://reviews.llvm.org/D120398#3351998>, @MyDeveloperDay wrote:
>
>> Before this lands can we have a discussion about what clarity this gives us?, because I think it makes the code more unreadable, but surely we are saving just 7x(3 bytes) (the difference between and int and a unsigned char for 7 enums)
>>
>> Is saving 21 bytes valuable?
>
> Peanut gallery says: I think giving every enum a concrete underlying type is a //good// practice, so I don't think this makes anything less readable. (That is, I'd even favor adding `: int` to each of these enums, over the status quo where the type is merely implied — we've seen that people can be unsure of the exact behavior.)

I also like a defined type for enums (I even have multiple enums based on bool).

> I can imagine that someone might object that hard-coding one byte for each enum might one day cause problems if we want more than 256 possible settings for any given enum, but that's not likely, is it?

I think an option with more than 256 variants would be a horror. But even if we need that, we can just increase the type, since we have no API or ABI stability in clang-format. We regulary change a bool to an enum.

That being said, I don't care that much that we declare the enums with a type. But I care about consistency. Either land this, or revert D93758 <https://reviews.llvm.org/D93758>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120398