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

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

"named"?

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

unittests/Format/FormatTest.cpp

8069

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

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

8072

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

8072

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.

docs/ClangFormatStyleOptions.rst

**BreakBeforeBraces** (``BraceBreakingStyle``)
   * ``BS_Stroustrup`` (in configuration: ``Stroustrup``)
     Like ``Attach``, but break before function definitions, and 'else'.
   * ``BS_Allman`` (in configuration: ``Allman``)
     Always break before braces.
   * ``BS_GNU`` (in configuration: ``GNU``)
     Always break before braces and add an extra level of indentation to
     braces of control statements, not to those of class, function
     or other definitions.
+  * ``BS_KernighanRitchie`` (in configuration: ``KernighanRitchie``)
+    Like ``Attach``, but break before braces on function definitions.
 
 
 **BreakBeforeTernaryOperators** (``bool``)
   If ``true``, ternary operators will be placed after line breaks.
 
 **BreakConstructorInitializersBeforeComma** (``bool``)
   Always break constructor initializers before commas and align
   the commas with the colon.

include/clang/Format/Format.h

enum BraceBreakingStyle {
     BS_Linux,
     /// Like \c Attach, but break before function definitions, and 'else'.
     BS_Stroustrup,
     /// Always break before braces.
     BS_Allman,
     /// Always break before braces and add an extra level of indentation to
     /// braces of control statements, not to those of class, function
     /// or other definitions.
-    BS_GNU
+    BS_GNU,
+    /// Like \c Attach, but break before braces on named function
+    /// definitions.
+    BS_KernighanRitchie
   };
 
   /// \brief The brace breaking style to use.
   BraceBreakingStyle BreakBeforeBraces;
 
   /// \brief If \c true, format braced lists as best suited for C++11 braced
   /// lists.
   ///

lib/Format/Format.cpp

 
 template <> struct ScalarEnumerationTraits<FormatStyle::BraceBreakingStyle> {
   static void enumeration(IO &IO, FormatStyle::BraceBreakingStyle &Value) {
     IO.enumCase(Value, "Attach", FormatStyle::BS_Attach);
     IO.enumCase(Value, "Linux", FormatStyle::BS_Linux);
     IO.enumCase(Value, "Stroustrup", FormatStyle::BS_Stroustrup);
     IO.enumCase(Value, "Allman", FormatStyle::BS_Allman);
     IO.enumCase(Value, "GNU", FormatStyle::BS_GNU);
+    IO.enumCase(Value, "KernighanRitchie", FormatStyle::BS_KernighanRitchie);
   }
 };
 
 template <>
 struct ScalarEnumerationTraits<FormatStyle::NamespaceIndentationKind> {
   static void enumeration(IO &IO,
                           FormatStyle::NamespaceIndentationKind &Value) {
     IO.enumCase(Value, "None", FormatStyle::NI_None);

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatTryCatchBraceStyles) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Attach;
   verifyFormat("try {\n"
                "  // something\n"
                "} catch (...) {\n"
                "  // something\n"
                "}",
                Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_KernighanRitchie;
+  verifyFormat("try {\n"
+               "  // something\n"
+               "} catch (...) {\n"
+               "  // something\n"
+               "}",
+               Style);
   Style.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
   verifyFormat("try {\n"
                "  // something\n"
                "}\n"
                "catch (...) {\n"
                "  // something\n"
                "}",
                Style);
TEST_F(FormatTest, CatchExceptionReferenceBinding) {
   verifyFormat("void f() {\n"
                "  try {\n"
                "  } catch (const Exception &e) {\n"
                "  }\n"
                "}\n",
                getLLVMStyle());
 }
 
+TEST_F(FormatTest, KernighanRitchieBraceBreaking) {
+  FormatStyle BreakBeforeBrace = getLLVMStyle();
+  BreakBeforeBrace.BreakBeforeBraces = FormatStyle::BS_KernighanRitchie;
+  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);
+
+  verifyFormat("void foo()\n"
+               "{\n"
+               "  if (a) {\n"
+               "    a();\n"
+               "  } else {\n"
+               "    b();\n"
+               "  }\n"
+               "}\n",
+               BreakBeforeBrace);
+
+  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);
+
+  verifyFormat("void f()\n"
+               "{\n"
+               "  for (int i = 0; i < 10; ++i) {\n"
+               "    a();\n"
+               "  }\n"
+               "  while (false) {\n"
+               "    b();\n"
+               "  }\n"
+               "  do {\n"
+               "    c();\n"
+               "  } while (false);\n"
+               "}\n",
+               BreakBeforeBrace);
+
+  verifyFormat("namespace ns {\n"
+               "class A {\n"
+               "public:\n"
+               "  void method(int x, int y) noexcept\n"
+               "  {\n"
+               "    try {\n"
+               "      return;\n"
+               "    } catch (...) {\n"
+               "      return;\n"
+               "    }\n"
+               "  }\n"
+               "};\n"
+               "}\n",
+               BreakBeforeBrace);
+
+  verifyFormat("void foo() {}\n"
+               "void bar()\n"
+               "#ifdef _DEBUG\n"
+               "{\n"
+               "  foo();\n"
+               "}\n"
+               "#else\n"
+               "{\n"
+               "}\n"
+               "#endif",
+               BreakBeforeBrace);
+
+  verifyFormat("void foobar() { int i = 5; }\n"
+               "#ifdef _DEBUG\n"
+               "void bar() {}\n"
+               "#else\n"
+               "void bar() { foobar(); }\n"
+               "#endif",
+               BreakBeforeBrace);
+}
+
 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)."));
CHECK_PARSE("BreakBeforeBraces: Attach", BreakBeforeBraces,
               FormatStyle::BS_Attach);
   CHECK_PARSE("BreakBeforeBraces: Linux", BreakBeforeBraces,
               FormatStyle::BS_Linux);
   CHECK_PARSE("BreakBeforeBraces: Stroustrup", BreakBeforeBraces,
               FormatStyle::BS_Stroustrup);
   CHECK_PARSE("BreakBeforeBraces: Allman", BreakBeforeBraces,
               FormatStyle::BS_Allman);
   CHECK_PARSE("BreakBeforeBraces: GNU", BreakBeforeBraces, FormatStyle::BS_GNU);
+  CHECK_PARSE("BreakBeforeBraces: KernighanRitchie", BreakBeforeBraces,
+              FormatStyle::BS_KernighanRitchie);
 
   Style.NamespaceIndentation = FormatStyle::NI_All;
   CHECK_PARSE("NamespaceIndentation: None", NamespaceIndentation,
               FormatStyle::NI_None);
   CHECK_PARSE("NamespaceIndentation: Inner", NamespaceIndentation,
               FormatStyle::NI_Inner);
   CHECK_PARSE("NamespaceIndentation: All", NamespaceIndentation,
               FormatStyle::NI_All);