[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 ↗

(On Diff #260551)

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

1768

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

1825

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

2291–2294

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

1767

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 ↗

(On Diff #260814)

? are you removing something?

2062 ↗

(On Diff #260814)

unrelated change

clang/lib/Format/Format.cpp

1143 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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

clang/lib/Format/Format.cpp

1143 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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 ↗

(On Diff #260814)

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

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1143 ↗

(On Diff #260814)

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/lib/Format/UnwrappedLineParser.cpp

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) {
       nextToken();
       return !HasError;
     }
     switch (FormatTok->Tok.getKind()) {
     case tok::caret:
       nextToken();
       if (FormatTok->is(tok::l_brace)) {
         parseChildBlock();
       }
       break;
case tok::semi:
       }
       HasError = true;
       if (!ContinueOnSemicolons)
         return !HasError;
       nextToken();
       break;
     case tok::comma:
       nextToken();
       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.BraceWrapping.AfterEnum)
+    addUnwrappedLine();
   // Parse enum body.
   nextToken();
+  if (Style.BraceWrapping.AfterEnum) {
+    addUnwrappedLine();
+    Line->Level += 1;
+  }
   bool HasError = !parseBracedList(/*ContinueOnSemicolons=*/true);
+  if (Style.BraceWrapping.AfterEnum)
+    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