Fixes bug 20587 - Add K&R break before braces style

[lifted]

Summary

http://llvm.org/bugs/show_bug.cgi?id=20587

Added K&R style. It could be enabled by the following option:

BreakBeforeBraces: KernighanRitchie

This style is like Attach, but break *only* before function
declarations.

As I can see, no additional logic required to support this style, any
style different from other styles automagically satisfies K&R.

[djasper]

LOL.. Very interesting that no additional logic is necessary.

Looks good with a few minor comments.

docs/ClangFormatStyleOptions.rst

176 ↗

(On Diff #12325)

Please generate this with docs/tools/dump_format_style.py. Otherwise, the next re-generation of this file will contain unrelated changes.

include/clang/Format/Format.h

287 ↗

(On Diff #12325)

"named"?

Also, maybe "..., but only break ..."?

unittests/Format/FormatTest.cpp

8088

Call this KRStyle?

[lifted]

• Regenerate ClangFormatStyleOptions.rst with a tool
• Fix description of the BS_KernighanRitchie
• Add a test case to check formatting of lambdas

[lifted]

Minor changes in Format.h to synchronize it with ClangFormatStyleOptions.rst.

[djasper]

I have done a bit more reading and now I am thoroughly confused:

1. I think Linux uses K&R style. So we should not need to introduce a new style but fix Linux style instead.
2. According to all resources I have found, K&R style also puts the "{" on the new line for classes and namespaces.
3. Neither Linux nor K&R style should but the "{" on the new line for structs, so we will need to handle structs and classes differently.

Does that make sense?

[lifted]

I think Linux uses K&R style. So we should not need to introduce a new style but fix Linux style instead.

Yep, that's what I wrote in the original bug. I don't mind fixing linux style. But it could break backward compatibility.

According to all resources I have found, K&R style also puts the "{" on the new line for classes and namespaces.

The only resource I've found is AStyle Manual.
Original K&R book contains no classes nor namespaces, so everyone is free to interpret K&R style for C++ constructs as they like. I wish I could ask Ritchie how would he place braces...
My opinion is that Stroustrup style is the closest one to original K&R style, it just adds a few more breaks for control structures.

we will need to handle structs and classes differently.

I don't think it's a good idea. From C++ point of view, struct is almost identical to class. If formatter changes the breaks when I replace struct with a class, it will confuse me much. I don't know anyone who wish to format structs and classes differently.

Possibly, it's better to provide fine-grained control over the brace usage (break after struct/class/) like Uncrucify do. The BreakBeforeBrace could then be high-level interface for fine-grained options.

include/clang/Format/Format.h

287 ↗

(On Diff #12325)

I used "named" here because I'm not sure how to format lambdas in K&R. I think it's perfectly legal to attach braces to lambda declarations because lambdas could be nested (unlike named functions).
Either way, comments for other styles don't stress this, so I'll remove "named".

[djasper]

Ok. I have also come to the conclusion that it is best to have one higher level style choice (I'd name this Linux style and fix it similar to what the K&R style from this patch does) and then give options to "override" each individual brace choice.

Can you fix the Linux style in this patch, basically making it what K&R style does now? We can then add the further options later or when requested. I actually do know codebases where structs are formatted with the brace attached whereas classes are not :-(.

[lifted]

Can you fix the Linux style in this patch, basically making it what K&R style does now?

Ok.

actually do know codebases where structs are formatted with the brace attached whereas classes are not :-(.

Possibly, they use AStyle for formatting :) I think fine-grained options is a good solution for such codebases. In addition, it's still trivial enough to provide "ArtisticStyleLinux" option to support the codebases.

[djasper]

unittests/Format/FormatTest.cpp

8091

Can you add small test for structs just so that we realize when we are changing the behavior there?

[djasper]

I have done the same for Kernel style and it seems the pretty consistency attach the "{" to structs.. So maybe we should just leave structs alone for now :-(.

[lifted]

Well, just to clarify:
Are we decided to follow AStyle way and break after classes and namespaces, but not before structs?

unittests/Format/FormatTest.cpp

8091

Ok, I'll add one.

[djasper]

I think so. Not sure whether it makes sense, but there are definitely arguments for it. astyle conformance is one, but more importantly structs are more of a C-concept and K&R style (well the book) is pretty consistent about attaching the "{" to the struct. Treating the C++ concepts of namespaces and classes differently seems inconsistent but what many projects do.

[lifted]

Fix Linux BrakeBeforeBraces style: attach left braces to structs as
shown to us by the prophets Kernighan and Ritchie.

Before:

namespace ns
{
class C
{
public:
  int x;
};
struct X
{
  int x;
};
}

After:

namespace ns
{
class C
{
public:
  int x;
};
struct X {
  int x;
};
}

[djasper]

Looks good.

[lifted]

Ouch, the arc commit used the original review summary and didn't allow me to edit the commit message :(
I'm really sorry about it.

lib/Format/UnwrappedLineParser.cpp

if (I->Tok->isNot(tok::period))
     return false;
   ++I;
   if (I->Tok->TokenText != "scope")
     return false;
   ++I;
   return I->Tok->is(tok::l_paren);
 }
 
+static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
+                                   const FormatToken &InitialToken) {
+  switch (Style.BreakBeforeBraces) {
+  case FormatStyle::BS_Linux:
+    return InitialToken.isOneOf(tok::kw_namespace, tok::kw_class);
+  case FormatStyle::BS_Allman:
+  case FormatStyle::BS_GNU:
+    return true;
+  default:
+    return false;
+  }
+}
+
 void UnwrappedLineParser::parseChildBlock() {
   FormatTok->BlockKind = BK_Block;
   nextToken();
   {
     bool GoogScope =
         Style.Language == FormatStyle::LK_JavaScript && IsGoogScope(*Line);
     ScopedLineState LineState(*this);
     ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
void UnwrappedLineParser::parseTryCatch() {
   }
   if (NeedsUnwrappedLine) {
     addUnwrappedLine();
   }
 }
 
 void UnwrappedLineParser::parseNamespace() {
   assert(FormatTok->Tok.is(tok::kw_namespace) && "'namespace' expected");
+
+  const FormatToken &InitialToken = *FormatTok;
   nextToken();
   if (FormatTok->Tok.is(tok::identifier))
     nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-    if (Style.BreakBeforeBraces == FormatStyle::BS_Linux ||
-        Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
-        Style.BreakBeforeBraces == FormatStyle::BS_GNU)
+    if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
     bool AddLevel = Style.NamespaceIndentation == FormatStyle::NI_All ||
                     (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
                      DeclarationScopeStack.size() > 1);
     parseBlock(/*MustBeDeclaration=*/true, AddLevel);
     // Munch the semicolon after a namespace. This is more common than one would
     // think. Puttin the semicolon into its own line is very ugly.
if (FormatTok->Tok.is(tok::l_brace)) {
     }
   }
   // We fall through to parsing a structural element afterwards, so that in
   // enum A {} n, m;
   // "} n, m;" will end up in one unwrapped line.
 }
 
 void UnwrappedLineParser::parseRecord() {
+  const FormatToken &InitialToken = *FormatTok;
   nextToken();
   if (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::kw___attribute,
                          tok::kw___declspec, tok::kw_alignas)) {
     nextToken();
     // We can have macros or attributes in between 'class' and the class name.
     if (FormatTok->Tok.is(tok::l_paren)) {
       parseParens();
     }
if (FormatTok->Tok.is(tok::colon) || FormatTok->Tok.is(tok::less)) {
       while (!eof() && FormatTok->Tok.isNot(tok::l_brace)) {
         if (FormatTok->Tok.is(tok::semi))
           return;
         nextToken();
       }
     }
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
-    if (Style.BreakBeforeBraces == FormatStyle::BS_Linux ||
-        Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
-        Style.BreakBeforeBraces == FormatStyle::BS_GNU)
+    if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
     parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
                /*MunchSemi=*/false);
   }
   // We fall through to parsing a structural element afterwards, so
   // class A {} n, m;
   // will end up in one unwrapped line.

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpaceBeforeAssignmentOperators) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpaceBeforeAssignmentOperators = false;
   verifyFormat("int a= 5;", Spaces);
   verifyFormat("a+= 42;", Spaces);
   verifyFormat("a or_eq 8;", Spaces);
 }
 
 TEST_F(FormatTest, LinuxBraceBreaking) {
-  FormatStyle BreakBeforeBrace = getLLVMStyle();
-  BreakBeforeBrace.BreakBeforeBraces = FormatStyle::BS_Linux;
+  FormatStyle LinuxBraceStyle = getLLVMStyle();
+  LinuxBraceStyle.BreakBeforeBraces = FormatStyle::BS_Linux;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"
                "{\n"
                "  void f()\n"
                "  {\n"
                "    if (true) {\n"
                "      a();\n"
                "      b();\n"
                "    }\n"
                "  }\n"
                "  void g() { return; }\n"
-               "}\n"
-               "}",
-               BreakBeforeBrace);
+               "};\n"
+               "struct B {\n"
+               "  int x;\n"
+               "};\n"
+               "}\n",
+               LinuxBraceStyle);
+  verifyFormat("enum X {\n"
+               "  Y = 0,\n"
+               "}\n",
+               LinuxBraceStyle);
+  verifyFormat("struct S {\n"
+               "  int Type;\n"
+               "  union {\n"
+               "    int x;\n"
+               "    double y;\n"
+               "  } Value;\n"
+               "  class C\n"
+               "  {\n"
+               "    MyFavoriteType Value;\n"
+               "  } Class;\n"
+               "}\n",
+               LinuxBraceStyle);
 }
 
 TEST_F(FormatTest, StroustrupBraceBreaking) {
-  FormatStyle BreakBeforeBrace = getLLVMStyle();
-  BreakBeforeBrace.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
+  FormatStyle StroustrupBraceStyle = getLLVMStyle();
+  StroustrupBraceStyle.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
   verifyFormat("namespace a {\n"
                "class A {\n"
                "  void f()\n"
                "  {\n"
                "    if (true) {\n"
                "      a();\n"
                "      b();\n"
                "    }\n"
                "  }\n"
                "  void g() { return; }\n"
-               "}\n"
-               "}",
-               BreakBeforeBrace);
+               "};\n"
+               "struct B {\n"
+               "  int x;\n"
+               "};\n"
+               "}\n",
+               StroustrupBraceStyle);
 
   verifyFormat("void foo()\n"
                "{\n"
                "  if (a) {\n"
                "    a();\n"
                "  }\n"
                "  else {\n"
                "    b();\n"
                "  }\n"
                "}\n",
-               BreakBeforeBrace);
+               StroustrupBraceStyle);
 
   verifyFormat("#ifdef _DEBUG\n"
                "int foo(int i = 0)\n"
                "#else\n"
                "int foo(int i = 5)\n"
                "#endif\n"
                "{\n"
                "  return i;\n"
                "}",
-               BreakBeforeBrace);
+               StroustrupBraceStyle);
 
   verifyFormat("void foo() {}\n"
                "void bar()\n"
                "#ifdef _DEBUG\n"
                "{\n"
                "  foo();\n"
                "}\n"
                "#else\n"
                "{\n"
                "}\n"
                "#endif",
-               BreakBeforeBrace);
+               StroustrupBraceStyle);
 
   verifyFormat("void foobar() { int i = 5; }\n"
                "#ifdef _DEBUG\n"
                "void bar() {}\n"
                "#else\n"
                "void bar() { foobar(); }\n"
                "#endif",
-               BreakBeforeBrace);
+               StroustrupBraceStyle);
 }
 
 TEST_F(FormatTest, AllmanBraceBreaking) {
-  FormatStyle BreakBeforeBrace = getLLVMStyle();
-  BreakBeforeBrace.BreakBeforeBraces = FormatStyle::BS_Allman;
+  FormatStyle AllmanBraceStyle = getLLVMStyle();
+  AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"
                "{\n"
                "  void f()\n"
                "  {\n"
                "    if (true)\n"
                "    {\n"
                "      a();\n"
                "      b();\n"
                "    }\n"
                "  }\n"
                "  void g() { return; }\n"
-               "}\n"
+               "};\n"
+               "struct B\n"
+               "{\n"
+               "  int x;\n"
+               "};\n"
                "}",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("void f()\n"
                "{\n"
                "  if (true)\n"
                "  {\n"
                "    a();\n"
                "  }\n"
                "  else if (false)\n"
                "  {\n"
                "    b();\n"
                "  }\n"
                "  else\n"
                "  {\n"
                "    c();\n"
                "  }\n"
                "}\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("void f()\n"
                "{\n"
                "  for (int i = 0; i < 10; ++i)\n"
                "  {\n"
                "    a();\n"
                "  }\n"
                "  while (false)\n"
                "  {\n"
                "    b();\n"
                "  }\n"
                "  do\n"
                "  {\n"
                "    c();\n"
                "  } while (false)\n"
                "}\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("void f(int a)\n"
                "{\n"
                "  switch (a)\n"
                "  {\n"
                "  case 0:\n"
                "    break;\n"
                "  case 1:\n"
                "  {\n"
                "    break;\n"
                "  }\n"
                "  case 2:\n"
                "  {\n"
                "  }\n"
                "  break;\n"
                "  default:\n"
                "    break;\n"
                "  }\n"
                "}\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("enum X\n"
                "{\n"
                "  Y = 0,\n"
                "}\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
   verifyFormat("enum X\n"
                "{\n"
                "  Y = 0\n"
                "}\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("@interface BSApplicationController ()\n"
                "{\n"
                "@private\n"
                "  id _extraIvar;\n"
                "}\n"
                "@end\n",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("#ifdef _DEBUG\n"
                "int foo(int i = 0)\n"
                "#else\n"
                "int foo(int i = 5)\n"
                "#endif\n"
                "{\n"
                "  return i;\n"
                "}",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("void foo() {}\n"
                "void bar()\n"
                "#ifdef _DEBUG\n"
                "{\n"
                "  foo();\n"
                "}\n"
                "#else\n"
                "{\n"
                "}\n"
                "#endif",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   verifyFormat("void foobar() { int i = 5; }\n"
                "#ifdef _DEBUG\n"
                "void bar() {}\n"
                "#else\n"
                "void bar() { foobar(); }\n"
                "#endif",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
   // This shouldn't affect ObjC blocks..
   verifyFormat("[self doSomeThingWithACompletionHandler:^{\n"
                "    // ...\n"
                "    int i;\n"
                "}];",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
   verifyFormat("void (^block)(void) = ^{\n"
                "    // ...\n"
                "    int i;\n"
                "};",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
   // .. or dict literals.
   verifyFormat("void f()\n"
                "{\n"
                "  [object someMethod:@{ @\"a\" : @\"b\" }];\n"
                "}",
-               BreakBeforeBrace);
+               AllmanBraceStyle);
 
-  BreakBeforeBrace.ColumnLimit = 19;
-  verifyFormat("void f() { int i; }", BreakBeforeBrace);
-  BreakBeforeBrace.ColumnLimit = 18;
+  AllmanBraceStyle.ColumnLimit = 19;
+  verifyFormat("void f() { int i; }", AllmanBraceStyle);
+  AllmanBraceStyle.ColumnLimit = 18;
   verifyFormat("void f()\n"
                "{\n"
                "  int i;\n"
                "}",
-               BreakBeforeBrace);
-  BreakBeforeBrace.ColumnLimit = 80;
+               AllmanBraceStyle);
+  AllmanBraceStyle.ColumnLimit = 80;
 
-  FormatStyle BreakBeforeBraceShortIfs = BreakBeforeBrace;
+  FormatStyle BreakBeforeBraceShortIfs = AllmanBraceStyle;
   BreakBeforeBraceShortIfs.AllowShortIfStatementsOnASingleLine = true;
   BreakBeforeBraceShortIfs.AllowShortLoopsOnASingleLine = true;
   verifyFormat("void f(bool b)\n"
                "{\n"
                "  if (b)\n"
                "  {\n"
                "    return;\n"
                "  }\n"
verifyFormat("void f() {\n"
                "  try {\n"
                "  } catch (const Exception &e) {\n"
                "  }\n"
                "}\n",
                getLLVMStyle());
 }
 
 TEST_F(FormatTest, UnderstandsPragmas) {
   verifyFormat("#pragma omp reduction(| : var)");
   verifyFormat("#pragma omp reduction(+ : var)");
 
   EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
             "(including parentheses).",
             format("#pragma    mark   Any non-hyphenated or hyphenated string "
                    "(including parentheses)."));
 }
 
 #define EXPECT_ALL_STYLES_EQUAL(Styles)                                        \
   for (size_t i = 1; i < Styles.size(); ++i)                                   \
     EXPECT_EQ(Styles[0], Styles[i]) << "Style #" << i << " of "                \