[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

1143

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

1769

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

1830

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
  };
}

2292–2299

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

1768

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

721

? are you removing something?

2062

unrelated change

clang/lib/Format/Format.cpp

1143

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

721

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.

2062

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

clang/lib/Format/Format.cpp

1143

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

721

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

1143

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

721

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

721

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

1143

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

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1143

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);
     }
 
-
-**InsertTrailingCommas** (``TrailingCommaStyle``) can be set to ``TCS_Wrapped``
-  to insert trailing commas in container literals (arrays and objects) that wrap
-  across multiple lines. It is currently only available for JavaScript and
-  disabled by default (``TCS_None``).
-
-  ``InsertTrailingCommas`` cannot be used together with ``BinPackArguments`` as
-  inserting the comma disables bin-packing.
-
-  .. code-block:: c++
-
-    TSC_Wrapped:
-    const someArray = [
-      aaaaaaaaaaaaaaaaaaaaaaaaaa,
-      aaaaaaaaaaaaaaaaaaaaaaaaaa,
-      aaaaaaaaaaaaaaaaaaaaaaaaaa,
-      //                        ^ inserted
-    ]
-
-
 **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,
**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) {
+         [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
-             callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
                  u = c;
              }]
      }
 
 **ObjCSpaceAfterProperty** (``bool``)
   Add a space after ``@property`` in Objective-C, i.e. use
   ``@property (readonly)`` instead of ``@property(readonly)``.
 

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) {

   enum UseTabStyle {
     /// Never use tab.
     UT_Never,
     /// Use tabs only for indentation.
     UT_ForIndentation,
     /// Fill all leading whitespace with tabs, and use spaces for alignment that
     /// appears within a line (e.g. consecutive assignments and declarations).
     UT_ForContinuationAndIndentation,
-    /// Use tabs for line continuation and indentation, and spaces for
-    /// alignemnt.
+    /// Use tabs for line continuation and indentation, and spaces for alignment.
     UT_AlignWithSpaces,
     /// Use tabs whenever we need to fill whitespace that spans at least from
     /// one tab stop to the next one.
     UT_Always
   };
 
   /// \brief Use ``\r\n`` instead of ``\n`` for line breaks.
   /// Also used as fallback if ``DeriveLineEnding`` is 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 = (Language != FormatStyle::LK_Cpp);
   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 = getMicrosoftStyle(FormatStyle::LK_Cpp);
+  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);