[clang-format] Fixed typedef enum brace wrapping

[PriMee]

Summary

Bug: https://bugs.llvm.org/show_bug.cgi?id=34016 - Typedef enum part

Problem:

Clang format does not allow the flag BraceWrapping.AfterEnum control the case when our enum is preceded by typedef keyword (what is common in C language).

Patch description:

Added case to the "AfterEnum" flag when our enum does not start a line - is preceded by typedef keyword.

After fix:

CONFIG:

BreakBeforeBraces: Custom
BraceWrapping: { 
AfterClass: true, AfterControlStatement: true, AfterEnum: true, AfterFunction: true, AfterNamespace: false, AfterStruct: true, AfterUnion: true, BeforeCatch: true, BeforeElse: true 
}

BEFORE:

typedef enum 
{
    a,
    b,
    c
} SomeEnum;

AFTER:

typedef enum 
{
    a,
    b,
    c
} SomeEnum;

[krasimir]

These look like they could be two separate patches: I like the typedef enum part, not convinced about the extern part. Also, please add unit tests.

[PriMee]

I merged them just because of their presence in the same bug report... As to the unit tests, I'll add them as soon as possible.

With reference to extern part, once again:
There is no brace wrapping flag that let us control opening brace's position. In case of other keywords (class, function, control statement etc.) we have opportunity to decide how should it look like. Here, we can't do it similarly. What we want is leaving braces unformatted (leave them as in the input), but what's more we still want to call parseBlock function. The only option is to set MustBreakBefore flag manually (only when needed, when the left brace is on non-header line, parseBlock does move it by default).

One more correction: Would be probably better to replace the line MustBreakBeforeNextToken = true;
with
FormatTok->MustBreakBefore = true;
It does work as expected as well (still passes every test - there are some!), but is more intuitive and understandable.

I'll work on these problems on monday.

[PriMee]

Unit tests added. If there is indeed a necessity to separate these two cases just inform me :)

[krasimir]

Thank you! I prefer this patch to be split in two. I am still not convinced about the extern part: some clients might prefer the other style.

[PriMee]

Done.
Extern C part moved to: https://reviews.llvm.org/D37260

[djasper]

Looks good. Thank you.

[PriMee]

I am glad to hear that. Would be great if someone could commit it. Thank You :)

[PriMee]

This comment was removed by PriMee.

lib/Format/TokenAnnotator.cpp

if ((Right.Previous->is(tok::l_brace) ||
     // Don't put enums or option definitions onto single lines in protocol
     // buffers.
     return true;
   }
   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)
     return true;
 
   if ((Style.Language == FormatStyle::LK_Java ||
        Style.Language == FormatStyle::LK_JavaScript) &&
       Left.is(TT_LeadingJavaAnnotation) &&

lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseStructuralElement() {
   case tok::kw___try:
     parseTryCatch();
     return;
   case tok::kw_extern:
     nextToken();
     if (FormatTok->Tok.is(tok::string_literal)) {
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
+        if (isOnNewLine(*FormatTok))
+          FormatTok->MustBreakBefore = true;
         parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
         addUnwrappedLine();
         return;
       }
     }
     break;
   case tok::kw_export:
     if (Style.Language == FormatStyle::LK_JavaScript) {

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsEnumTypes) {
   verifyFormat("enum X : int {\n"
                "  A, // Force multiple lines.\n"
                "  B\n"
                "};");
   verifyFormat("enum X : int { A, B };");
   verifyFormat("enum X : std::uint32_t { A, B };");
 }
 
+TEST_F(FormatTest, FormatsTypedefEnum) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 40;
+  verifyFormat("typedef enum {} EmptyEnum;");
+  verifyFormat("typedef enum { A, B, C } ShortEnum;");
+  verifyFormat("typedef enum {\n"
+               "  ZERO = 0,\n"             
+               "  ONE = 1,\n"
+               "  TWO = 2,\n"
+               "  THREE = 3\n"
+               "} LongEnum;",
+               Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
+  verifyFormat("typedef enum {} EmptyEnum;");
+  verifyFormat("typedef enum { A, B, C } ShortEnum;");
+  verifyFormat("typedef enum\n"
+               "{\n"
+               "  ZERO = 0,\n"             
+               "  ONE = 1,\n"
+               "  TWO = 2,\n"
+               "  THREE = 3\n"
+               "} LongEnum;",
+               Style);
+}
+
 TEST_F(FormatTest, FormatsNSEnums) {
   verifyGoogleFormat("typedef NS_ENUM(NSInteger, SomeName) { AAA, BBB }");
   verifyGoogleFormat("typedef NS_ENUM(NSInteger, MyType) {\n"
                      "  // Information about someDecentlyLongValue.\n"
                      "  someDecentlyLongValue,\n"
                      "  // Information about anotherDecentlyLongValue.\n"
                      "  anotherDecentlyLongValue,\n"
                      "  // Information about aThirdDecentlyLongValue.\n"
EXPECT_EQ("namespace out { namespace mid { namespace in {\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace mid\n"
                    "} // namespace out",
                    Style));
 }
 
-TEST_F(FormatTest, FormatsExternC) { verifyFormat("extern \"C\" {\nint a;"); }
+TEST_F(FormatTest, FormatsExternC) { 
+  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {};");
+  EXPECT_EQ("extern \"C\" {\n"
+            "int i = 42;\n"
+            "}",
+            format("extern \"C\" {\n"
+                   "int i = 42;\n"
+                   "}"));
+  EXPECT_EQ("extern \"C\"\n" 
+            "{\n"
+            "int i = 42;\n"
+            "}",
+            format("extern \"C\"\n" 
+                   "{\n"
+                   "int i = 42;\n"
+                   "}"));
+    
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
   verifyFormat(
       "asm(\"movq\\t%%rbx, %%rsi\\n\\t\"\n"
       "    \"cpuid\\n\\t\"\n"
       "    \"xchgq\\t%%rbx, %%rsi\\n\\t\"\n"