[clang-format] Fix Microsoft style for enums

[asmith]

Summary

Before this change enums were formatted incorrectly for the Microsoft style.

[C++ Example]

enum {
  one,
  two
} three, four;

[Incorrectly Formatted]

enum
{
  one,
  two
} three,
    four;

[Correct Format with Patch]

enum
{
  one,
  two
} three, four;

[asmith]

This change tries to fix this bug.
https://bugs.llvm.org/show_bug.cgi?id=32810

[asmith]

Fix line breaks like this:

enum { a=1, b=2, c=3
} myEnum;

which becomes:

enum
{

a=1,
b=2,
c=3

} myEnum;

[MyDeveloperDay]

Before looking into the merit of the change, it needs a couple of things

• This needs unit tests in clang/unittests/Format to demonstrate the problem and test this fixing it
• This also needs the ClangStyleOptions.rst file generated with the clang/tools/dump_style.py
• We should add a release note too

[MyDeveloperDay]

So it doesn't overcome the trailing "," issue. (which is sometimes used to put enums on multiple lines) - (NOTE: maybe it shouldn't)

---
Language: Cpp
BasedOnStyle: LLVM
AllowEnumsOnASingleLine: true

enum
{
  B,
  C,
  D,
} A, B;

vs

enum { B, C, D } A, B;

[MyDeveloperDay]

I think the name should be AllowShortEnumsOnASingleLine

As once the enum goes over a certain width it will revert to as it was before.

enum { EOF, VOID, CHAR, SHORT, DOT, NUMBER, IDENTIFIER } A;

vs

enum {
  EOF,
  VOID,
  CHAR,
  SHORT,
  INT,
  LONG,
  SIGNED,
  UNSIGNED,
  BOOL,
  FLOAT,
  DOUBLE,
  COMPLEX,
  CONST,
  RESTRICT,
  VOLATILE,
  ATOMIC,
  STRUCT,
  UNION,
  ENUM,
  LPAREN,
  RPAREN,
  LBRACKET,
  RBRACKET,
  ASTERISK,
  DOT,
  NUMBER,
  IDENTIFIER
} A;

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1145

You'll need to fix up the C# tests

C:/llvm-project/clang/unittests/Format/FormatTestCSharp.cpp(48): error:       Expected: Code.str()
      Which is: "public enum var { none, @string, bool, @enum }"
To be equal to: format(Code, Style)
      Which is: "public enum var\n{\n    none,\n    @string,\n    bool,\n    @enum\n}"
With diff:
@@ -1,1 +1,7 @@
-public enum var { none, @string, bool, @enum }
+public enum var
+{
+    none,
+    @string,
+    bool,
+    @enum
+}

I think that is ok as most of the documentation seems to show them not on a single line

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/enum

clang/lib/Format/UnwrappedLineParser.cpp

1790

see below, I think you need to know this braced Initialization is part of a enum

1851

I'm wondering if this is going to impact other braced constructs?

void main()
{
    std::vector<int> list = { 1,2,3};
}

becomes

void main() {
  std::vector<int> list = {1,
  2,
  3
  };
}

2313–2320

I think you are going to have to pass down that this BracedList is for a enum only

[asmith]

Rename option
Add test case
Only apply to Microsoft CPP style

[asmith]

clang/lib/Format/UnwrappedLineParser.cpp

1789

This and the line below are probably applying to more than enums. Any suggestions on a better fix?

[asmith]

In D78982#2007172, @MyDeveloperDay wrote:

• This needs unit tests in clang/unittests/Format to demonstrate the problem and test this fixing it

Is there an easy way to check the test is working?

'ninja check-clang-format' passes but would like to verify.

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

719–720

? are you removing something?

2094–2095

unrelated change

clang/lib/Format/Format.cpp

1145

I think its ok to leave it was, just if up the unit tests

(the fact you didn't see this suggests you are not running the unit tests please ensure you are)

[asmith]

The is done unless there are other comments.

clang/docs/ClangFormatStyleOptions.rst

719–720

These are changes from running the python script to regenerate the docs. Someone forgot to run the script. This info is no longer in the header.

2094–2095

Not my change. The column is too long so looks like someone manually edited the file.

clang/lib/Format/Format.cpp

1145

You need to use the latest changes. The is only applied when the style is Msft and C++. See the GetMicrosoftStyle() method.

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

719–720

If a change isn't related to your feature, simply ignore it, but don't include other changes in yours. If this has truly gone then we need to handle that separately

clang/lib/Format/Format.cpp

1145

I don't think you should change the style based on the language here, either on or off, but not changing based on language because what about "ObjectiveC"?

As I added the Microsoft style to allow C# development I think its ok to set this to what you originally had, but fix up the tests

[asmith]

clang/docs/ClangFormatStyleOptions.rst

719–720

This is generated by a script. I don't think it's a good idea to edit an automatically generated file. I can exclude the change to the RST completely and someone can regenerate the RST in a follow on patch.

[MyDeveloperDay]

see D79095: [clang-format] NFC Correct clang-format headers file so documentation can be once again autogenerated this should be handled separately.

clang/docs/ClangFormatStyleOptions.rst

719–720

This is from D73768: clang-format: [JS] document InsertTrailingCommas. we may need to determine why the code was not put into Format.h but that can be handled by a separate revision

[asmith]

Enable for C#

[asmith]

clang/lib/Format/Format.cpp

1145

Ive enabled this for C#. I don't see the connection between ObjectiveC and Microsoft Style.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1145

The Microsoft style was a style we added to try and give us a unique style to setup for C#, but that wouldn't stop a user for using it for other languages

Saying that you want a style for C++ but not other C based languges feels a little odd

Its perfectly acceptable for someone using Objective-C to use the BasedOnStyle of Microsoft

as such I think its better for a style to have a set default, then let users modify from the default.

[MyDeveloperDay]

Sorry please adjust the patch to remove the un-related changes

[MyDeveloperDay]

You will need to rebase as the non related rst changes have been handled by D79095: [clang-format] NFC Correct clang-format headers file so documentation can be once again autogenerated

[MyDeveloperDay]

LGTM

[asmith]

In D78982#2012992, @MyDeveloperDay wrote:

LGTM

Thanks for the help!

[RKSimon]

@asmith I'm seeing MSVC warnings from this patch:

E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1475): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'
E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1828): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'

[curdeius]

In D78982#2013221, @RKSimon wrote:

@asmith I'm seeing MSVC warnings from this patch:

E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1475): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'
E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1828): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'

That's caused by the changed signature of parseBracedList and this change hasn't been taken into account at the above mentioned locations:

- bool parseBracedList(bool ContinueOnSemicolons = false,
+  bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false, tok::TokenKind ClosingBraceKind = tok::r_brace);

BTW, it's a bit strange that a new parameter had been added in the middle of parameter list.

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
     switch (a) {                    vs.     switch (a) {
     case 1: x = 1; break;                   case 1:
     case 2: return;                           x = 1;
     }                                         break;
                                             case 2:
                                               return;
                                             }
 
+**AllowShortEnumsOnASingleLine** (``bool``)
+  Allow short enums on a single line.
+
+  .. code-block:: c++
+
+    true:
+    enum { A, B } myEnum;
+
+    false:
+    enum
+    {
+      A,
+      B
+    } myEnum;
+
 **AllowShortFunctionsOnASingleLine** (``ShortFunctionStyle``)
   Dependent on the value, ``int f() { return 0; }`` can be put on a
   single line.
 
   Possible values:
 
   * ``SFS_None`` (in configuration: ``None``)
     Never merge functions into a single line.
.. code-block:: c++
     }
 
     false:
     void f() {
       f(aaaaaaaaaaaaaaaaaaaa,
         aaaaaaaaaaaaaaaaaaaa,
         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
     }
 
 **BinPackParameters** (``bool``)
   If ``false``, a function declaration's or function definition's
   parameters will either all be on the same line or will have one line each.
 
   .. code-block:: c++
 
     true:
     void f(int aaaaaaaaaaaaaaaaaaaa, int aaaaaaaaaaaaaaaaaaaa,
            int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
**ObjCBreakBeforeNestedBlockParam** (``bool``)
   Break parameters list into lines when there is nested block
   parameters in a fuction call.
 
   .. code-block:: c++
 
     false:
      - (void)_aMethod
      {
          [self.test1 t:self w:self callback:^(typeof(self) self, NSNumber
          *u, NSNumber *v) {
              u = c;
          }]
      }
      true:
      - (void)_aMethod
      {
         [self.test1 t:self
                      w:self

clang/include/clang/Format/Format.h

struct FormatStyle {
   ///   void myFunction(int a,
   ///                   int b,
   ///                   int c,
   ///                   int d,
   ///                   int e);
   /// \endcode
   bool AllowAllParametersOfDeclarationOnNextLine;
 
+  /// Allow short enums on a single line.
+  /// \code
+  ///   true:
+  ///   enum { A, B } myEnum;
+  ///
+  ///   false:
+  ///   enum
+  ///   {
+  ///     A,
+  ///     B
+  ///   } myEnum;
+  /// \endcode
+  bool AllowShortEnumsOnASingleLine;
+
   /// Different styles for merging short blocks containing at most one
   /// statement.
   enum ShortBlockStyle {
     /// Never merge blocks into a single line.
     /// \code
     ///   while (true) {
     ///   }
     ///   while (true) {
return AccessModifierOffset == R.AccessModifierOffset &&
            AlignEscapedNewlines == R.AlignEscapedNewlines &&
            AlignOperands == R.AlignOperands &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllArgumentsOnNextLine == R.AllowAllArgumentsOnNextLine &&
            AllowAllConstructorInitializersOnNextLine ==
                R.AllowAllConstructorInitializersOnNextLine &&
            AllowAllParametersOfDeclarationOnNextLine ==
                R.AllowAllParametersOfDeclarationOnNextLine &&
+           AllowShortEnumsOnASingleLine == R.AllowShortEnumsOnASingleLine &&
            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==
                R.AllowShortIfStatementsOnASingleLine &&
            AllowShortLambdasOnASingleLine == R.AllowShortLambdasOnASingleLine &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AlignOperands", Style.AlignOperands);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllArgumentsOnNextLine",
                    Style.AllowAllArgumentsOnNextLine);
     IO.mapOptional("AllowAllConstructorInitializersOnNextLine",
                    Style.AllowAllConstructorInitializersOnNextLine);
     IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
                    Style.AllowAllParametersOfDeclarationOnNextLine);
+    IO.mapOptional("AllowShortEnumsOnASingleLine",
+                   Style.AllowShortEnumsOnASingleLine);
     IO.mapOptional("AllowShortBlocksOnASingleLine",
                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortLambdasOnASingleLine",
                    Style.AllowShortLambdasOnASingleLine);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlignOperands = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
   LLVMStyle.AlignConsecutiveMacros = false;
   LLVMStyle.AllowAllArgumentsOnNextLine = true;
   LLVMStyle.AllowAllConstructorInitializersOnNextLine = true;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
+  LLVMStyle.AllowShortEnumsOnASingleLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
   Style.BraceWrapping.AfterFunction = true;
   Style.BraceWrapping.AfterNamespace = true;
   Style.BraceWrapping.AfterObjCDeclaration = true;
   Style.BraceWrapping.AfterStruct = true;
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.BeforeCatch = true;
   Style.BraceWrapping.BeforeElse = true;
   Style.PenaltyReturnTypeOnItsOwnLine = 1000;
+  Style.AllowShortEnumsOnASingleLine = false;
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   Style.AllowShortCaseLabelsOnASingleLine = false;
   Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   Style.AllowShortLoopsOnASingleLine = false;
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   return Style;
 }

clang/lib/Format/UnwrappedLineParser.h

private:
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
   void parseStructuralElement();
   bool tryToParseBracedList();
-  bool parseBracedList(bool ContinueOnSemicolons = false,
+  bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false,
                        tok::TokenKind ClosingBraceKind = tok::r_brace);
   void parseParens();
   void parseSquare(bool LambdaIntroducer = false);
   void parseIfThenElse();
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);

clang/lib/Format/UnwrappedLineParser.cpp

bool UnwrappedLineParser::tryToParseBracedList() {
   if (FormatTok->BlockKind == BK_Block)
     return false;
   nextToken();
   parseBracedList();
   return true;
 }
 
 bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
+                                          bool IsEnum,
                                           tok::TokenKind ClosingBraceKind) {
   bool HasError = false;
 
   // FIXME: Once we have an expression parser in the UnwrappedLineParser,
   // replace this by using parseAssigmentExpression() inside.
   do {
     if (Style.isCSharp()) {
       if (FormatTok->is(TT_JsFatArrow)) {
if (Style.Language == FormatStyle::LK_JavaScript) {
       if (FormatTok->is(tok::l_brace)) {
         // Could be a method inside of a braced list `{a() { return 1; }}`.
         if (tryToParseBracedList())
           continue;
         parseChildBlock();
       }
     }
     if (FormatTok->Tok.getKind() == ClosingBraceKind) {
+      if (IsEnum && !Style.AllowShortEnumsOnASingleLine)
+        addUnwrappedLine();
       nextToken();
       return !HasError;
     }
     switch (FormatTok->Tok.getKind()) {
     case tok::caret:
       nextToken();
       if (FormatTok->is(tok::l_brace)) {
         parseChildBlock();
case tok::semi:
       }
       HasError = true;
       if (!ContinueOnSemicolons)
         return !HasError;
       nextToken();
       break;
     case tok::comma:
       nextToken();
+      if (IsEnum && !Style.AllowShortEnumsOnASingleLine)
+        addUnwrappedLine();
       break;
     default:
       nextToken();
       break;
     }
   } while (!eof());
   return false;
 }
 
if (Style.Language == FormatStyle::LK_Java) {
     parseJavaEnumBody();
     return true;
   }
   if (Style.Language == FormatStyle::LK_Proto) {
     parseBlock(/*MustBeDeclaration=*/true);
     return true;
   }
 
+  if (!Style.AllowShortEnumsOnASingleLine)
+    addUnwrappedLine();
   // Parse enum body.
   nextToken();
-  bool HasError = !parseBracedList(/*ContinueOnSemicolons=*/true);
+  if (!Style.AllowShortEnumsOnASingleLine) {
+    addUnwrappedLine();
+    Line->Level += 1;
+  }
+  bool HasError = !parseBracedList(/*ContinueOnSemicolons=*/true,
+                                   /*IsEnum=*/true);
+  if (!Style.AllowShortEnumsOnASingleLine)
+    Line->Level -= 1;
   if (HasError) {
     if (FormatTok->is(tok::semi))
       nextToken();
     addUnwrappedLine();
   }
   return true;
 
   // There is no addUnwrappedLine() here so that we fall through to parsing a

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, CaseRanges) {
   verifyFormat("switch (x) {\n"
                "case 'A' ... 'Z':\n"
                "case 1 ... 5:\n"
                "case a ... b:\n"
                "  break;\n"
                "}");
 }
 
+TEST_F(FormatTest, ShortEnums) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = true;
+  verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum\n"
+               "{\n"
+               "  A,\n"
+               "  B,\n"
+               "  C\n"
+               "} ShortEnum1, ShortEnum2;",
+               Style);
+}
+
 TEST_F(FormatTest, ShortCaseLabels) {
   FormatStyle Style = getLLVMStyle();
   Style.AllowShortCaseLabelsOnASingleLine = true;
   verifyFormat("switch (a) {\n"
                "case 1: x = 1; break;\n"
                "case 2: return;\n"
                "case 3:\n"
                "case 4:\n"
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
   CHECK_PARSE_BOOL(AlignConsecutiveMacros);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllConstructorInitializersOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
+  CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);

clang/unittests/Format/FormatTestCSharp.cpp

 }
 
 TEST_F(FormatTestCSharp, CSharpRegions) {
   verifyFormat("#region aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaa "
                "aaaaaaaaaaaaaaa long region");
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var { none, @string, bool, @enum }");
+  verifyFormat("public enum var\n"
+               "{\n"
+               "    none,\n"
+               "    @string,\n"
+               "    bool,\n"
+               "    @enum\n"
+               "}");
 }
 
 TEST_F(FormatTestCSharp, CSharpNullCoalescing) {
   verifyFormat("var test = ABC ?? DEF");
   verifyFormat("string myname = name ?? \"ABC\";");
   verifyFormat("return _name ?? \"DEF\";");
 }