Clang-format: Add Whitesmiths indentation style

[timwoj]

Summary

This patch adds support for the Whitesmiths indentation style to clang-format. It’s an update to a patch submitted in 2015 (D6833), but reworks it to use the newer API.

There are still some issues with this patch, primarily around switch and case support. The added unit test won’t currently pass because of the remaining issues.

[echristo]

Martin should know who should look at this... maybe Krasimir?

[mboehme]

In D67627#1671638, @echristo wrote:

Martin should know who should look at this... maybe Krasimir?

Yes, I think he'd be a good person to look at this.

[MyDeveloperDay]

Thank you for this patch

This LGTM, ensure you comment out the failing unit tests and add a FIXME to the test as to why they don't currently work (or fix them if possible), the tests need to keep passing or others will not know they haven't broken things.

Normally there always would be a comment saying please state a public style guide that uses a new style, but as Whitesmiths style is mentioned on the Wikipedia page about Indentation style so I would think you were good to add it!

[MyDeveloperDay]

when/if you commit can you abandon D6833: Clang-format: Braces Indent Style Whitesmith

[krasimir]

Adding @djasper as this is really a re-implementation of https://reviews.llvm.org/D6833.
Agree with @MyDeveloperDay that in general we should aim to document all the known cases where a style doesn't work (with FIXMEs etc.), but not submit failing tests. We can have tests that demonstrate the current (bad) behavior together with a FIXME comment about what is the expected good outcome instead.

[timwoj]

Marked failing tests as FIXME

[MyDeveloperDay]

LGTM

[timwoj]

I don't have push access for master. Can someone merge this?

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
       {
       }
       enum X : int
       {
         A,
         B
       };
 
+  * ``BS_Whitesmiths`` (in configuration: ``Whitesmiths``)
+    Like ``Allman`` but always indent braces and line up code with braces.
+
+    .. code-block:: c++
+
+      try
+        {
+        foo();
+        }
+      catch ()
+        {
+        }
+      void foo() { bar(); }
+      class foo
+        {
+        };
+      if (foo())
+        {
+        }
+      else
+        {
+        }
+      enum X : int
+        {
+        A,
+        B
+        };
+
   * ``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.
 
     .. code-block:: c++
 
       try

clang/include/clang/Format/Format.h

enum BraceBreakingStyle {
     ///     {
     ///     }
     ///   enum X : int
     ///   {
     ///     A,
     ///     B
     ///   };
     /// \endcode
+    BS_Whitesmiths,
+    /// Like ``Allman`` but always indent braces and line up code with braces.
+    /// \code
+    ///    try
+    ///     {
+    ///     foo();
+    ///     }
+    ///   catch ()
+    ///     {
+    ///     }
+    ///   void foo() { bar(); }
+    ///   class foo
+    ///     {
+    ///     };
+    ///   if (foo())
+    ///     {
+    ///     }
+    ///   else
+    ///     {
+    ///     }
+    ///   enum X : int
+    ///     {
+    ///     A,
+    ///     B
+    ///     };
+    /// \endcode
     BS_GNU,
     /// Like ``Attach``, but break before functions.
     /// \code
     ///   try {
     ///     foo();
     ///   } catch () {
     ///   }
     ///   void foo() { bar(); }

clang/lib/Format/ContinuationIndenter.cpp

if (!NextNonComment)
     NextNonComment = &Current;
 
   // Java specific bits.
   if (Style.Language == FormatStyle::LK_Java &&
       Current.isOneOf(Keywords.kw_implements, Keywords.kw_extends))
     return std::max(State.Stack.back().LastSpace,
                     State.Stack.back().Indent + Style.ContinuationIndentWidth);
 
+  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths &&
+      State.Line->First->is(tok::kw_enum))
+    return (Style.IndentWidth * State.Line->First->IndentLevel) +
+           Style.IndentWidth;
+
   if (NextNonComment->is(tok::l_brace) && NextNonComment->BlockKind == BK_Block)
     return Current.NestingLevel == 0 ? State.FirstIndent
                                      : State.Stack.back().Indent;
   if ((Current.isOneOf(tok::r_brace, tok::r_square) ||
        (Current.is(tok::greater) &&
         (Style.Language == FormatStyle::LK_Proto ||
          Style.Language == FormatStyle::LK_TextProto))) &&
       State.Stack.size() > 1) {

clang/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, "Mozilla", FormatStyle::BS_Mozilla);
     IO.enumCase(Value, "Stroustrup", FormatStyle::BS_Stroustrup);
     IO.enumCase(Value, "Allman", FormatStyle::BS_Allman);
+    IO.enumCase(Value, "Whitesmiths", FormatStyle::BS_Whitesmiths);
     IO.enumCase(Value, "GNU", FormatStyle::BS_GNU);
     IO.enumCase(Value, "WebKit", FormatStyle::BS_WebKit);
     IO.enumCase(Value, "Custom", FormatStyle::BS_Custom);
   }
 };
 
 template <>
 struct ScalarEnumerationTraits<FormatStyle::BreakConstructorInitializersStyle> {
case FormatStyle::BS_Allman:
     Expanded.BraceWrapping.AfterNamespace = true;
     Expanded.BraceWrapping.AfterObjCDeclaration = true;
     Expanded.BraceWrapping.AfterStruct = true;
     Expanded.BraceWrapping.AfterUnion = true;
     Expanded.BraceWrapping.AfterExternBlock = true;
     Expanded.BraceWrapping.BeforeCatch = true;
     Expanded.BraceWrapping.BeforeElse = true;
     break;
+  case FormatStyle::BS_Whitesmiths:
+    Expanded.BraceWrapping.AfterCaseLabel = true;
+    Expanded.BraceWrapping.AfterClass = true;
+    Expanded.BraceWrapping.AfterControlStatement = true;
+    Expanded.BraceWrapping.AfterEnum = true;
+    Expanded.BraceWrapping.AfterFunction = true;
+    Expanded.BraceWrapping.AfterNamespace = true;
+    Expanded.BraceWrapping.AfterObjCDeclaration = true;
+    Expanded.BraceWrapping.AfterStruct = true;
+    Expanded.BraceWrapping.AfterExternBlock = true;
+    Expanded.BraceWrapping.BeforeCatch = true;
+    Expanded.BraceWrapping.BeforeElse = true;
+    break;
   case FormatStyle::BS_GNU:
     Expanded.BraceWrapping = {true, true, true, true, true, true, true, true,
                               true, true, true, true, true, true, true, true};
     break;
   case FormatStyle::BS_WebKit:
     Expanded.BraceWrapping.AfterFunction = true;
     break;
   default:

clang/lib/Format/UnwrappedLineFormatter.cpp

void UnwrappedLineFormatter::formatFirstToken(
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
     Newlines = std::min(1u, Newlines);
 
   if (Newlines)
     Indent = NewlineIndent;
 
+  // If in Whitemsmiths mode, indent start and end of blocks
+  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
+    if (RootToken.isOneOf(tok::l_brace, tok::r_brace, tok::kw_case))
+      Indent += Style.IndentWidth;
+  }
+
   // Preprocessor directives get indented before the hash only if specified
   if (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
       (Line.Type == LT_PreprocessorDirective ||
        Line.Type == LT_ImportStatement))
     Indent = 0;
 
   Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
                                  Line.InPPDirective &&

clang/unittests/Format/FormatTest.cpp

verifyFormat("try\n"
                "{\n"
                "  // something\n"
                "}\n"
                "catch (...)\n"
                "{\n"
                "  // something\n"
                "}",
                Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Whitesmiths;
+  verifyFormat("try\n"
+               "  {\n"
+               "  // something white\n"
+               "  }\n"
+               "catch (...)\n"
+               "  {\n"
+               "  // something white\n"
+               "  }",
+               Style);
   Style.BreakBeforeBraces = FormatStyle::BS_GNU;
   verifyFormat("try\n"
                "  {\n"
                "    // something\n"
                "  }\n"
                "catch (...)\n"
                "  {\n"
                "    // something\n"
TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
 
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   verifyFormat("void someLongFunction(\n"
                "    int someLongParameter) const\n"
                "{\n"
                "}",
                Style);
 
+  Style.BreakBeforeBraces = FormatStyle::BS_Whitesmiths;
+  verifyFormat("void someLongFunction(\n"
+               "    int someLongParameter) const\n"
+               "  {\n"
+               "  }",
+               Style);
+
   // Unless these are unknown annotations.
   verifyFormat("void SomeFunction(aaaaaaaaaa aaaaaaaaaaaaaaa,\n"
                "                  aaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                "    LONG_AND_UGLY_ANNOTATION;");
 
   // Breaking before function-like trailing annotations is fine to keep them
   // close to their arguments.
   verifyFormat("void aaaaaaaaaaaa(int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
verifyFormat("void f(bool b)\n"
                "  while (b)\n"
                "  {\n"
                "    return;\n"
                "  }\n"
                "}\n",
                BreakBeforeBraceShortIfs);
 }
 
+TEST_F(FormatTest, WhitesmithsBraceBreaking) {
+  FormatStyle WhitesmithsBraceStyle = getLLVMStyle();
+  WhitesmithsBraceStyle.BreakBeforeBraces = FormatStyle::BS_Whitesmiths;
+
+  // Make a few changes to the style for testing purposes
+  WhitesmithsBraceStyle.AllowShortFunctionsOnASingleLine =
+      FormatStyle::SFS_Empty;
+  WhitesmithsBraceStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+  WhitesmithsBraceStyle.ColumnLimit = 0;
+
+  // FIXME: this test case can't decide whether there should be a blank line
+  // after the ~D() line or not. It adds one if one doesn't exist in the test
+  // and it removes the line if one exists.
+  /*
+  verifyFormat("class A;\n"
+               "namespace B\n"
+               "  {\n"
+               "class C;\n"
+               "// Comment\n"
+               "class D\n"
+               "  {\n"
+               "public:\n"
+               "  D();\n"
+               "  ~D() {}\n"
+               "private:\n"
+               "  enum E\n"
+               "    {\n"
+               "    F\n"
+               "    }\n"
+               "  };\n"
+               "  } // namespace B\n",
+               WhitesmithsBraceStyle);
+  */
+
+  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()\n"
+               "    {\n"
+               "    return;\n"
+               "    }\n"
+               "  };\n"
+               "struct B\n"
+               "  {\n"
+               "  int x;\n"
+               "  };\n"
+               "  } // namespace a",
+               WhitesmithsBraceStyle);
+
+  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",
+               WhitesmithsBraceStyle);
+
+  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",
+               WhitesmithsBraceStyle);
+
+  // FIXME: the block and the break under case 2 in this test don't get indented correctly
+  /*
+  verifyFormat("void switchTest1(int a)\n"
+               "  {\n"
+               "  switch (a)\n"
+               "    {\n"
+               "    case 2:\n"
+               "      {\n"
+               "      }\n"
+               "      break;\n"
+               "    }\n"
+               "  }\n",
+               WhitesmithsBraceStyle);
+  */
+
+  // FIXME: the block and the break under case 2 in this test don't get indented correctly
+  /*
+  verifyFormat("void switchTest2(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",
+               WhitesmithsBraceStyle);
+  */
+
+  verifyFormat("enum X\n"
+               "  {\n"
+               "  Y = 0, // testing\n"
+               "  }\n",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("enum X\n"
+               "  {\n"
+               "  Y = 0\n"
+               "  }\n",
+               WhitesmithsBraceStyle);
+  verifyFormat("enum X\n"
+               "  {\n"
+               "  Y = 0,\n"
+               "  Z = 1\n"
+               "  };\n",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("@interface BSApplicationController ()\n"
+               "  {\n"
+               "@private\n"
+               "  id _extraIvar;\n"
+               "  }\n"
+               "@end\n",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("#ifdef _DEBUG\n"
+               "int foo(int i = 0)\n"
+               "#else\n"
+               "int foo(int i = 5)\n"
+               "#endif\n"
+               "  {\n"
+               "  return i;\n"
+               "  }",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("void foo() {}\n"
+               "void bar()\n"
+               "#ifdef _DEBUG\n"
+               "  {\n"
+               "  foo();\n"
+               "  }\n"
+               "#else\n"
+               "  {\n"
+               "  }\n"
+               "#endif",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("void foobar()\n"
+               "  {\n"
+               "  int i = 5;\n"
+               "  }\n"
+               "#ifdef _DEBUG\n"
+               "void bar()\n"
+               "  {\n"
+               "  }\n"
+               "#else\n"
+               "void bar()\n"
+               "  {\n"
+               "  foobar();\n"
+               "  }\n"
+               "#endif",
+               WhitesmithsBraceStyle);
+
+  // This shouldn't affect ObjC blocks..
+  verifyFormat("[self doSomeThingWithACompletionHandler:^{\n"
+               "  // ...\n"
+               "  int i;\n"
+               "}];",
+               WhitesmithsBraceStyle);
+  verifyFormat("void (^block)(void) = ^{\n"
+               "  // ...\n"
+               "  int i;\n"
+               "};",
+               WhitesmithsBraceStyle);
+  // .. or dict literals.
+  verifyFormat("void f()\n"
+               "  {\n"
+               "  [object someMethod:@{@\"a\" : @\"b\"}];\n"
+               "  }",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("int f()\n"
+               "  { // comment\n"
+               "  return 42;\n"
+               "  }",
+               WhitesmithsBraceStyle);
+
+  FormatStyle BreakBeforeBraceShortIfs = WhitesmithsBraceStyle;
+  BreakBeforeBraceShortIfs.AllowShortIfStatementsOnASingleLine =
+      FormatStyle::SIS_Always;
+  BreakBeforeBraceShortIfs.AllowShortLoopsOnASingleLine = true;
+  verifyFormat("void f(bool b)\n"
+               "  {\n"
+               "  if (b)\n"
+               "    {\n"
+               "    return;\n"
+               "    }\n"
+               "  }\n",
+               BreakBeforeBraceShortIfs);
+  verifyFormat("void f(bool b)\n"
+               "  {\n"
+               "  if (b) return;\n"
+               "  }\n",
+               BreakBeforeBraceShortIfs);
+  verifyFormat("void f(bool b)\n"
+               "  {\n"
+               "  while (b)\n"
+               "    {\n"
+               "    return;\n"
+               "    }\n"
+               "  }\n",
+               BreakBeforeBraceShortIfs);
+}
+
 TEST_F(FormatTest, GNUBraceBreaking) {
   FormatStyle GNUBraceStyle = getLLVMStyle();
   GNUBraceStyle.BreakBeforeBraces = FormatStyle::BS_GNU;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"
                "{\n"
                "  void f()\n"
TEST_F(FormatTest, ParsesConfiguration) {
   CHECK_PARSE("BreakBeforeBraces: Linux", BreakBeforeBraces,
               FormatStyle::BS_Linux);
   CHECK_PARSE("BreakBeforeBraces: Mozilla", BreakBeforeBraces,
               FormatStyle::BS_Mozilla);
   CHECK_PARSE("BreakBeforeBraces: Stroustrup", BreakBeforeBraces,
               FormatStyle::BS_Stroustrup);
   CHECK_PARSE("BreakBeforeBraces: Allman", BreakBeforeBraces,
               FormatStyle::BS_Allman);
+  CHECK_PARSE("BreakBeforeBraces: Whitesmiths", BreakBeforeBraces,
+              FormatStyle::BS_Whitesmiths);
   CHECK_PARSE("BreakBeforeBraces: GNU", BreakBeforeBraces, FormatStyle::BS_GNU);
   CHECK_PARSE("BreakBeforeBraces: WebKit", BreakBeforeBraces,
               FormatStyle::BS_WebKit);
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
               FormatStyle::BS_Custom);
 
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
   CHECK_PARSE("AlwaysBreakAfterReturnType: None", AlwaysBreakAfterReturnType,