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

[kwk]

Summary

Analogous to https://reviews.llvm.org/D93758, this patch uses
unsigned char for all enums in FormatStyle.

[kwk]

Rebased on upstream/main

[MyDeveloperDay]

Is there a clang-tidy check for this? I mean if we are doing this because it helps reduce the size of FormatStyle and its recommended practice for small enums, then we should have a clang-tidy check that catches us on this. (no?)

[curdeius]

Looks innocent but I have a few questions.
What's the size difference that you observe?
Any runtime penalty? (Usually compilers choose a faster underlying type for enums, not smaller)
Also, is this change very relevant to clang-format? We should not copy FormatStyle a lot, no?

[HazardyKnusperkeks]

The decision is made in the header (so I think it will always be int until this doesn't fit). To decide what is faster the compiler would need to know all the usages. Which it can't.

We have multiple times bit fields in clang-format to reduce the object size. I think it's only consistent to decrease the FormatStyle size.

The only reason I didn't do it for the enums introduced by me is consistency.

In D120398#3347726, @MyDeveloperDay wrote:

Is there a clang-tidy check for this? I mean if we are doing this because it helps reduce the size of FormatStyle and its recommended practice for small enums, then we should have a clang-tidy check that catches us on this. (no?)

None that I'm aware of.

[• Quuxplusone]

Any runtime penalty? (Usually compilers choose a faster underlying type for enums, not smaller)

The decision is made in the header (so I think it will always be int until this doesn't fit). To decide what is faster the compiler would need to know all the usages. Which it can't.

Yes, Clang chooses int by default. In fact, C++11 and later mandate that a new-style scoped enum enum class Foo { means the same thing as enum class Foo : int { — see https://timsong-cpp.github.io/cppwp/n3337/dcl.enum#5 . For unscoped old-style enums, Clang/GCC/MSVC all choose int by default, unless -fshort-enums is passed on the command line and/or the enum is annotated with __attribute__((packed)), in which case it chooses the smallest possible type. -fshort-enums and __attribute__((packed)) have no effect on new-style scoped enums nor on enums with explicit underlying types.

[MyDeveloperDay]

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?

[• Quuxplusone]

In 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 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?
Re space savings, OP says that they're saving 21 bytes per object and that they have "lots" of objects in their particular use-case, even if clang-format itself doesn't.

[MyDeveloperDay]

In my own area I gave FormatStyle a copy constructor and made it private, it hardly caused any issues, this isn't something that we tend copy about very much, we tend copy construct it only when we do getLLVMStyle() to make another style say like google style sure we'll do that a handful of times but I very much doubt it would make an impact.. can we measure?

I have no problems adding the base class if that's the LLVM coding style recommendation, otherwise I don't see it gives us anything.

[HazardyKnusperkeks]

In D120398#3352024, @Quuxplusone wrote:

In 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.

[kwk]

In D120398#3352436, @HazardyKnusperkeks wrote:

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.

Consistency was more or less the only reason why I made this change in the first place and I didn't expect so many comments on a change that is almost identical to D93758. Sure I didn't have a good argument for or against it so far but... consistency... I'm glad this discussion came up anyways. Will look at each comment tomorrow.

[owenpan]

Can we use int8_t instead of unsigned char to keep the enum types signed (and update the changes made in D93758)?

[HazardyKnusperkeks]

In D120398#3353053, @owenpan wrote:

Can we use int8_t instead of unsigned char to keep the enum types signed (and update the changes made in D93758)?

+1

[kwk]

I abandon this revision in favor of https://reviews.llvm.org/D120884.

clang/include/clang/Format/Format.h

struct FormatStyle {
   /// If ``true``, horizontally aligns arguments after an open bracket.
   ///
   /// This applies to round brackets (parentheses), angle brackets and square
   /// brackets.
   /// \version 3.8
   BracketAlignmentStyle AlignAfterOpenBracket;
 
   /// Different style for aligning array initializers.
-  enum ArrayInitializerAlignmentStyle {
+  enum ArrayInitializerAlignmentStyle : unsigned char {
     /// Align array column and left justify the columns e.g.:
     /// \code
     ///   struct test demo[] =
     ///   {
     ///       {56, 23,    "hello"},
     ///       {-1, 93463, "world"},
     ///       {7,  5,     "!!"   }
     ///   };
struct FormatStyle {
   ArrayInitializerAlignmentStyle AlignArrayOfStructures;
 
   /// Styles for alignment of consecutive tokens. Tokens can be assignment signs
   /// (see
   /// ``AlignConsecutiveAssignments``), bitfield member separators (see
   /// ``AlignConsecutiveBitFields``), names in declarations (see
   /// ``AlignConsecutiveDeclarations``) or macro definitions (see
   /// ``AlignConsecutiveMacros``).
-  enum AlignConsecutiveStyle {
+  enum AlignConsecutiveStyle : unsigned char {
     ACS_None,
     ACS_Consecutive,
     ACS_AcrossEmptyLines,
     ACS_AcrossComments,
     ACS_AcrossEmptyLinesAndComments
   };
 
   /// Style of aligning consecutive macro definitions.

   ///     AfterEnum: true
   ///     AfterStruct: false
   ///     SplitEmptyFunction: false
   /// \endcode
   /// \version 3.8
   BraceWrappingFlags BraceWrapping;
 
   /// Different ways to break before concept declarations.
-  enum BreakBeforeConceptDeclarationsStyle {
+  enum BreakBeforeConceptDeclarationsStyle : unsigned char {
     /// Keep the template declaration line together with ``concept``.
     /// \code
     ///   template <typename T> concept C = ...;
     /// \endcode
     BBCDS_Never,
     /// Breaking between template declaration and ``concept`` is allowed. The
     /// actual behavior depends on the content and line breaking rules and
     /// penalities.

   ///    // CommentPragmas: '^ FOOBAR pragma:'
   ///    // Will leave the following line unaffected
   ///    #include <vector> // FOOBAR pragma: keep
   /// \endcode
   /// \version 3.7
   std::string CommentPragmas;
 
   /// Different specifiers and qualifiers alignment styles.
-  enum QualifierAlignmentStyle {
+  enum QualifierAlignmentStyle : unsigned char {
     /// Don't change specifiers/qualifiers to either Left or Right alignment
     /// (default).
     /// \code
     ///    int const a;
     ///    const int *a;
     /// \endcode
     QAS_Leave,
     /// Change specifiers/qualifiers to be left-aligned.
///    NS_TABLE_.*_END$"
   ///           - 'cpp'
   ///         BasedOnStyle: llvm
   ///         CanonicalDelimiter: 'cc'
   /// \endcode
   /// \version 6
   std::vector<RawStringFormat> RawStringFormats;
 
   /// \brief The ``&`` and ``&&`` alignment style.
-  enum ReferenceAlignmentStyle {
+  enum ReferenceAlignmentStyle : unsigned char {
     /// Align reference like ``PointerAlignment``.
     RAS_Pointer,
     /// Align reference to the left.
     /// \code
     ///   int& a;
     /// \endcode
     RAS_Left,
     /// Align reference to the right.
///    NS_TABLE_.*_END$"
   ///   }
   /// \endcode
   /// \version 14
   bool RemoveBracesLLVM;
 
   /// \brief The possible positions for the requires clause. The
   /// ``IndentRequires`` option is only used if the ``requires`` is put on the
   /// start of a line.
-  enum RequiresClausePositionStyle {
+  enum RequiresClausePositionStyle : unsigned char {
     /// Always put the ``requires`` clause on its own line.
     /// \code
     ///   template <typename T>
     ///   requires C<T>
     ///   struct Foo {...
     ///
     ///   template <typename T>
     ///   requires C<T>
enum RequiresClausePositionStyle : unsigned char {
     RCPS_SingleLine,
   };
 
   /// \brief The position of the ``requires`` clause.
   /// \version 15
   RequiresClausePositionStyle RequiresClausePosition;
 
   /// \brief The style if definition blocks should be separated.
-  enum SeparateDefinitionStyle {
+  enum SeparateDefinitionStyle : unsigned char {
     /// Leave definition blocks as they are.
     SDS_Leave,
     /// Insert an empty line between definition blocks.
     SDS_Always,
     /// Remove any empty line between definition blocks.
     SDS_Never
   };