Extend format with AllowShortEnumsOnASingleLine option

[koalo]

Summary

Before, clang-format has tried to put enums on a single line whenever possible (unless in styles where the opening brace was put on a seperate line anyway).
AllowShortEnumsOnASingleLine that defaults to true (matching the conventional behaviour) can be set to false to force enums on multiple lines.

This feature is requested from time to time, e.g. here, and should be unobtrusive for users that do not set this option to false.

[MyDeveloperDay]

sorry searching through old issues, are you still interested in this patch?

[koalo]

In D54628#1742781, @MyDeveloperDay wrote:

sorry searching through old issues, are you still interested in this patch?

Yes, would be nice to have either this or an equivalent possibility that implements the same feature.

[MyDeveloperDay]

I'm thinking this is the same as BraceWrapping.AfterEnum, if you think your use case is covered would you consider Abandoning this revision so we know this functionality exists already, if not let work out what is missing

if (Right.is(TT_InlineASMBrace))
  return Right.HasUnescapedNewline;
if (isAllmanBrace(Left) || isAllmanBrace(Right))
  return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
         (Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
          Style.BraceWrapping.AfterEnum) ||
         (Line.startsWith(tok::kw_class) && Style.BraceWrapping.AfterClass) ||
         (Line.startsWith(tok::kw_struct) && Style.BraceWrapping.AfterStruct);
if (Left.is(TT_ObjCBlockLBrace) &&
    Style.AllowShortBlocksOnASingleLine == FormatStyle::SBS_Never)
  return true;

[koalo]

The difference is that BraceWrapping.AfterEnum also wraps before the brace.

With AfterEnum we have the following

true:
enum X : int
{
  B
};

false:
enum X : int { B };

But what I want is this:

enum X : int {
  B
};

[MyDeveloperDay]

It feels like it should somehow be following BreakBeforeBraces style but understand it might not

[koalo]

Yes it does not. Therefore, I have followed the same pattern as the the other AllowShort* options.

[MyDeveloperDay]

In D54628#1744108, @koalo wrote:

Yes it does not. Therefore, I have followed the same pattern as the the other AllowShort* options.

I'm not completely sure, but I don't think its quite the same, the AllowShort* options are working on merging simple blocks with tryMergeSimpleBlock(), either small or empty blocks

I think what you are really adding here is "AlwaysBreakAfterEnum" but is there a use case for having

typedef enum Color { Red, Blue, Green }; on one line but
typedef enum Color { Red, Blue, Green, Orange, Purple, Magenta, Cyan, Yellow }; on multiple

i.e. what do we define as "Short" here?

With the following config

---
Language: Cpp
BaseOnStyle: LLVM
BreakBeforeBraces: Custom
BraceWrapping:
    AfterEnum: true

You get (which is the same as GNU and Allman style)

enum
{
  Red,
  Blue,
  Green
} Color;

enum Color
{
  Red,
  Blue,
  Green,
  Orange,
  Purple,
  Magenta,
  Cyan,
  Yellow
};

false, as you said will put them on one line..

If you want the brace cuddled with the enum, like below, then we need something else, either your option or perhaps changing the AfterEnum to be a enum of Alway=true,Never=false,Attach or something like that

enum {
  Red,
  Blue,
  Green
} Color;

It is the "Short" word that seems a little confusing unless we are going to define "Short" as being a certain number of enumerations etc.. and that feels a bit clunky.

For the Attach option, I think we have to tell this above clause to break only for isAllman(Left) and not isAllman(Right) so the newline comes after the { not before.

Could I suggest you rebase,first, then it would be easier to try some stuff out.

But I certainly think your request is really to add the BS_Attach capability to enums without having the entire enum on the same line

Again sorry you had to wait so long, I am trying to work my way through the backlog (but it's going to take some time unless I can get some others to help me especially in getting things reviewed we decide to fix ;-) )

[MyDeveloperDay]

Ironically the LLVM style guide says enumerators should be written out as you requested, but from what I can tell its not actually possible to get that format without this sort of change, do you agree?

[koalo]

Rebased to current master

[koalo]

Yes, that is at least my understanding. I just rebased to master.

In my understanding, "short" means "put it on a single line if it fits considering the current maximum line length".

[MyDeveloperDay]

Thanks for rebasing, I think this is a good idea I'm just not sure about how the option presents itself, would you consider changing it?

clang/lib/Format/TokenAnnotator.cpp

3109

would you be happy to consider this being something like this instead?

if (isAllmanBrace(Left) && Style.BraceWrapping.AfterEnum == FormatStyle::BWAE_Attach &...

There is now precedent for the BraceWrapping styles to be an enum (see work by @mitchell-stellar who did something similar for AfterControlStylement)

[MyDeveloperDay]

In D54628#1745645, @koalo wrote:

Yes, that is at least my understanding. I just rebased to master.

In my understanding, "short" means "put it on a single line if it fits considering the current maximum line length".

If you are going to use AllowShortEnumsOnASingleLine you need to test it being true and where AfterEnum = true

[mitchell-stellar]

clang/lib/Format/TokenAnnotator.cpp

3109

I would not recommend making this option an enum. Style.BraceWrapping.AfterControlStatement is a special case since there was a potential readability issue for multi-line control statement conditions. Enums do not suffer from this. I think the option is in line with the other AllowShort*OnASingleLine options.

[MyDeveloperDay]

ok sounds good, @koalo do you have the tests to ensure AfterEnum=true and AllowShortEnumsOnASingleIne=true show on a single line? otherwise perhaps we are already good.

[MyDeveloperDay]

Nit: you should use a full context diff.

clang/docs/ClangFormatStyleOptions.rst

   If ``true``, ``while (true) continue;`` can be put on a single
   line.
 
+**AllowShortEnumsOnASingleLine** (``bool``)
+  Allows contracting enums to a single line.
+
+  E.g., this allows ``enum {A, B, C};`` to be put on a single line.
+
 **AlwaysBreakAfterDefinitionReturnType** (``DefinitionReturnTypeBreakingStyle``)
   The function definition return type breaking style to use.  This
   option is **deprecated** and is retained for backwards compatibility.

clang/include/clang/Format/Format.h

   /// line.
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``enum {A, B, C};`` can be put on a single line.
+  bool AllowShortEnumsOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards compatibility.
   enum DefinitionReturnTypeBreakingStyle {

clang/lib/Format/Format.cpp

                    Style.AllowShortIfStatementsOnASingleLine);
     IO.mapOptional("AllowShortLoopsOnASingleLine",
                    Style.AllowShortLoopsOnASingleLine);
+    IO.mapOptional("AllowShortEnumsOnASingleLine",
+                   Style.AllowShortEnumsOnASingleLine);
     IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
                    Style.AlwaysBreakAfterDefinitionReturnType);
     IO.mapOptional("AlwaysBreakAfterReturnType",

   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortEnumsOnASingleLine = true;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;

clang/lib/Format/TokenAnnotator.cpp

   }
   if (Right.is(TT_InlineASMBrace))
     return Right.HasUnescapedNewline;
+  if (isAllmanBrace(Left) && !Style.AllowShortEnumsOnASingleLine &&
+      (Line.startsWith(tok::kw_enum) ||
+       Line.startsWith(tok::kw_typedef, tok::kw_enum)))
+    return true;
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
     return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
            (Line.startsWith(tok::kw_typedef, tok::kw_enum) &&

clang/unittests/Format/FormatTest.cpp

   verifyFormat("enum ShortEnum { A, B, C };");
   verifyGoogleFormat("enum ShortEnum { A, B, C };");
 
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum Enum {\n"
+               "};",Style);
+  verifyFormat("enum {\n"
+               "};",Style);
+  verifyFormat("enum X E {\n"
+               "} d;",Style);
+  verifyFormat("enum __attribute__((...)) E {\n"
+               "} d;",Style);
+  verifyFormat("enum __declspec__((...)) E {\n"
+               "} d;",Style);
+  verifyFormat("enum ShortEnum {\n"
+               "  A,\n"
+               "  B,\n"
+               "  C\n"
+               "};",Style);
+
   EXPECT_EQ("enum KeepEmptyLines {\n"
             "  ONE,\n"
             "\n"

   verifyFormat("enum struct __attribute__((...)) E {} d;");
   verifyFormat("enum struct __declspec__((...)) E {} d;");
   verifyFormat("enum struct X f() {\n  a();\n  return 42;\n}");
+
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum struct Enum {\n"
+               "};",Style);
+  verifyFormat("enum struct {\n"
+               "};",Style);
+  verifyFormat("enum struct X E {\n"
+               "} d;",Style);
+  verifyFormat("enum struct __attribute__((...)) E {\n"
+               "} d;",Style);
+  verifyFormat("enum struct __declspec__((...)) E {\n"
+               "} d;",Style);
 }
 
 TEST_F(FormatTest, FormatsEnumClass) {

   verifyFormat("enum class __attribute__((...)) E {} d;");
   verifyFormat("enum class __declspec__((...)) E {} d;");
   verifyFormat("enum class X f() {\n  a();\n  return 42;\n}");
+
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum class Enum {\n"
+               "};",Style);
+  verifyFormat("enum class {\n"
+               "};",Style);
+  verifyFormat("enum class X E {\n"
+               "} d;",Style);
+  verifyFormat("enum class __attribute__((...)) E {\n"
+               "} d;",Style);
+  verifyFormat("enum class __declspec__((...)) E {\n"
+               "} d;",Style);
 }
 
 TEST_F(FormatTest, FormatsEnumTypes) {

                "};");
   verifyFormat("enum X : int { A, B };");
   verifyFormat("enum X : std::uint32_t { A, B };");
+
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum X : int {\n"
+               "  A,\n"
+         "  B\n"
+         "};",Style);
+  verifyFormat("enum X : std::uint32_t {\n"
+               "  A,\n"
+         "  B\n"
+         "};",Style);
 }
 
 TEST_F(FormatTest, FormatsTypedefEnum) {

                "  THREE = 3\n"
                "} LongEnum;",
                Style);
+
+  Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("typedef enum {\n"
+               "} EmptyEnum;",Style);
+  verifyFormat("typedef enum {\n"
+               "  A,\n"
+         "  B,\n"
+         "  C\n"
+         "} ShortEnum;",Style);
 }
 
 TEST_F(FormatTest, FormatsNSEnums) {