Clang-format: Braces Indent Style Whitesmith

[JVApen]

Summary

On request of Daniel,
the cleaned up patch.

For now, most of the stuff in the test works; and even running clang format on a sample code base gives some nice results.

Anyhow, the current issue still is that fixing the indent of the enumeration breaks other code.

I've also introduced the concept of a LevelGuard, since I got the feeling that doing the exact opposite of the increment of Line->Level bloats the code;
however is has not yet been aligned for the other styles. The position of those guards is currently one which is working; not yet the ideal one

[klimek]

Wouldn't it be simpler to not change the line level, and instead put an additional indent in for opening and closing braces?

[djasper]

I agree that that should lead to an way simpler implementation. It has the downside of spreading the brace handling stuff more outside of the UnwrappedLineParser, but that is probably ok in this case.

So, we should remove almost all the changes to the UnwrappedLineParser and instead modify UnwrappedLineFormatter::formatFirstToken(). JVApen, can you make those changes? Alternatively, I can also pick up and finish the patch.

[JVApen]

I'm gonna assume you guys now the code base better then I do :p
By just indenting the braces, its gonna most likely be much cleaner code,
though then all code in UnwrappedLineParser will be the exact same for
Allman and Whitesmiths.
Any ideas how to make sure this will stay in sync in the future?

Anyhow, for now I'm gonna play a bit more with the code trying to do it the
'easy way' and get familiar with the code some more.
It won't hurt since I've noticed that I also need another feature to be
compliant with our current style sheet.
If it wouldn't work out I'll send a cry and otherwise a patch.

JVApen

[JVApen]

I've fixed the method to do the indentation, by adapting indent in ContinuationIndenter::getNewLineColumn()

As far as I understand it, the indent of the content of the enumeration is determined by the very last statement.
In a first attempt, I looked at the current token being an identifier; though it was not selective enough.
Now I look at the previous token, though this worked for the current tests, it gives incorrect indent for the second enum-value.

Further more this introduces an issue with the breaks, where:
switch (i)
..{
case A:
..{
..}
..break;
..}

​becomes:​

​switch (i)
..{
case A:
..{
..}
break;
..}

I'm really starting to get lost in this one function call :s

[insysion]

Is this issue dead? I'm looking to use the tool for a Whitesmiths formatted project and would be happy to finish off the work.

[djasper]

In case you still want to move forward with this, some more comments.

Why have you opted for doing this in getNewLineColumn instead of the UnwrappedLineFormatter::formatFirstToken() function I suggested? I think the latter would be way easier, but there might be cases I am not thinking of right now.

docs/ClangFormatStyleOptions.rst

260

indented?

lib/Format/ContinuationIndenter.h

270

No. I don't think it is necessary to add an extra value to the indent state for this and doing so would be costly in runtime.

[timwoj]

I updated this patch to remove all of the code from ContinuationIndenter and to use the newer BraceWrapping style option instead of setting each one individually in UnwrappedLineParser.

I still have the same issue with enums. Looking at the RootToken in UnwrappedLineFormatter::formatFirstToken when parsing an enum, the first pass through it has the entire line for the enum in it with all of the newlines stripped from my test case. It then doesn't ever return the braces as separate lines. If someone could give me a pointer as to how/why enums are tokenized differently from everything else, I can likely fix it. The test case I'm using looks like:

verifyFormat("enum X\n"
             "  {\n"
             "  Y = 0,\n"
             "  }\n",
             WhitesmithsBraceStyle);

I also have the issue with a break after a block inside of a case statement, as mentioned in https://reviews.llvm.org/D6833#107539. Other than those two, it's mostly working correctly. I removed the test for lambdas because I'm not entirely sure how that should even be formatted with Whitesmiths, but I can add it back in again.

Should I update this thread with the new diff against HEAD, or should I open a new one and close this one as dead?

docs/ClangFormatStyleOptions.rst

     Like ``Attach``, but break before function definitions, and 'else'.
   * ``BS_Allman`` (in configuration: ``Allman``)
     Always break before braces.
+  * ``BS_Whitesmiths`` (in configuration: ``Whitesmiths``)
+    Like ``Allman``, with the braces intended too.
   * ``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

include/clang/Format/Format.h

     BS_Stroustrup,
     /// Always break before braces.
     BS_Allman,
+    /// Allman style with a level of indentation before the braces
+    BS_Whitesmiths,
     /// 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.

lib/Format/ContinuationIndenter.h

         NestedNameSpecifierContinuation(0), CallContinuation(0), VariablePos(0),
         ContainsLineBreak(false), ContainsUnwrappedBuilder(0),
         AlignColons(true), ObjCSelectorNameFound(false),
-        HasMultipleNestedBlocks(false), NestedBlockInlined(false) {}
+        HasMultipleNestedBlocks(false), NestedBlockInlined(false),
+        BlockNoExtraIndent(false) {}
 
   /// \brief The position to which a specific parenthesis level needs to be
   /// indented.

   // "function" in JavaScript) is not wrapped to a new line.
   bool NestedBlockInlined;
 
+  //
+  bool BlockNoExtraIndent;
+
   bool operator<(const ParenState &Other) const {
     if (Indent != Other.Indent)
       return Indent < Other.Indent;

lib/Format/ContinuationIndenter.cpp

     return std::max(State.Stack.back().LastSpace,
                     State.Stack.back().Indent + Style.ContinuationIndentWidth);
 
-  if (NextNonComment->is(tok::l_brace) && NextNonComment->BlockKind == BK_Block)
+  // Start of braced block
+  if (NextNonComment->is(tok::l_brace) &&
+      NextNonComment->BlockKind == BK_Block) {
+    // Whitesmiths indents the { and } too, compared to Allman,
+    // this also needs to happen for the special parsed blocks.
+    if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+      return (Current.NestingLevel == 0 ? State.FirstIndent
+                                        : State.Stack.back().Indent) +
+             Style.IndentWidth;
     return Current.NestingLevel == 0 ? State.FirstIndent
                                      : State.Stack.back().Indent;
+  }
+  // End of braced block (or ])
   if (Current.isOneOf(tok::r_brace, tok::r_square)) {
     if (Current.closesBlockTypeList(Style))
       return State.Stack[State.Stack.size() - 2].NestedBlockIndent;

     // Ensure that we fall back to the continuation indent width instead of
     // just flushing continuations left.
     return State.Stack.back().Indent + Style.ContinuationIndentWidth;
+
+  // Whitesmiths indents the { and } too, compared to Allman,
+  // this also needs to happen for the special parsed blocks.
+  // The content between the braces needs to no extra indent
+  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
+    if (PreviousNonComment && PreviousNonComment->is(tok::l_brace) &&
+        PreviousNonComment->BlockKind == BK_Block)
+      return State.Stack.back().Indent - Style.IndentWidth;
+  }
   return State.Stack.back().Indent;
 }
 

lib/Format/Format.cpp

     IO.enumCase(Value, "Linux", FormatStyle::BS_Linux);
     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);
   }
 };

lib/Format/TokenAnnotator.cpp

             Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline);
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
     return Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+           Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
            Style.BreakBeforeBraces == FormatStyle::BS_GNU;
   if (Style.Language == FormatStyle::LK_Proto && Left.isNot(tok::l_brace) &&
       Right.is(TT_SelectorName))

lib/Format/UnwrappedLineFormatter.cpp

     if (Limit == 0)
       return 0;
     if ((Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+         Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
          Style.BreakBeforeBraces == FormatStyle::BS_GNU) &&
         (I[1]->First->is(tok::l_brace) && !Style.AllowShortBlocksOnASingleLine))
       return 0;

   if (PreviousLine && PreviousLine->First->isAccessSpecifier())
     Newlines = std::min(1u, Newlines);
 
+  if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
+    // Setting the indent level for Whitesmiths is implemented similar to
+    // Allman. e.g. Getting the indentation correct except for the { and }
+    // tokens. The correction of those tokens is done here.
+    if (RootToken.isOneOf(tok::l_brace, tok::r_brace))
+      Indent += Style.IndentWidth;
+  }
   Whitespaces->replaceWhitespace(RootToken, Newlines, IndentLevel, Indent,
                                  Indent, InPPDirective &&
                                              !RootToken.HasUnescapedNewline);

lib/Format/UnwrappedLineParser.cpp

   CompoundStatementIndenter(UnwrappedLineParser *Parser,
                             const FormatStyle &Style, unsigned &LineLevel)
       : LineLevel(LineLevel), OldLineLevel(LineLevel) {
-    if (Style.BreakBeforeBraces == FormatStyle::BS_Allman) {
+    if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
       Parser->addUnwrappedLine();
     } else if (Style.BreakBeforeBraces == FormatStyle::BS_GNU) {
       Parser->addUnwrappedLine();

   case FormatStyle::BS_Linux:
     return InitialToken.isOneOf(tok::kw_namespace, tok::kw_class);
   case FormatStyle::BS_Allman:
+  case FormatStyle::BS_Whitesmiths:
   case FormatStyle::BS_GNU:
     return true;
   default:

     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU) {
       addUnwrappedLine();
     } else {

     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
         Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
       addUnwrappedLine();

     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
         Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
       addUnwrappedLine();

   if (FormatTok->Tok.is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
-    if (Style.BreakBeforeBraces == FormatStyle::BS_GNU)
+    if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
+        Style.BreakBeforeBraces == FormatStyle::BS_GNU)
       addUnwrappedLine();
   } else {
     addUnwrappedLine();

     if (FormatTok->Tok.is(tok::kw_break)) {
       // "break;" after "}" on its own line only for BS_Allman and BS_GNU
       if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+          Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
           Style.BreakBeforeBraces == FormatStyle::BS_GNU) {
         addUnwrappedLine();
       }

 
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
+        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU)
       addUnwrappedLine();
     parseBlock(/*MustBeDeclaration=*/true);

unittests/Format/FormatTest.cpp

                "  // something\n"
                "}",
                Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Whitesmiths;
+  verifyFormat("try\n"
+               "  {\n"
+               "  // something\n"
+               "  }\n"
+               "catch (...)\n"
+               "  {\n"
+               "  // something\n"
+               "  }",
+               Style);
   Style.BreakBeforeBraces = FormatStyle::BS_GNU;
   verifyFormat("try\n"
                "  {\n"

   // they are not function-like.
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 47;
+
   verifyFormat("void someLongFunction(\n"
                "    int someLoooooooooooooongParameter) const {\n}",
                getLLVMStyleWithColumns(47));

                "}",
                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"

                LinuxBraceStyle);
   verifyFormat("enum X {\n"
                "  Y = 0,\n"
+               "};\n",
+               LinuxBraceStyle);
+  verifyFormat("enum X {\n"
+               "  Y = 0,\n"
                "}\n",
                LinuxBraceStyle);
   verifyFormat("struct S {\n"

                "  Y = 0\n"
                "}\n",
                AllmanBraceStyle);
+  verifyFormat("enum X\n"
+               "{\n"
+               "  Y = 0\n"
+               "};\n",
+               AllmanBraceStyle);
 
   verifyFormat("@interface BSApplicationController ()\n"
                "{\n"

                BreakBeforeBraceShortIfs);
 }
 
+TEST_F(FormatTest, WhitesmithsBraceBreaking) {
+  FormatStyle WhitesmithsBraceStyle = getLLVMStyle();
+  WhitesmithsBraceStyle.BreakBeforeBraces = FormatStyle::BS_Whitesmiths;
+
+  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"
+               "  }\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() { return; }\n"
+               "  };\n"
+               "struct B\n"
+               "  {\n"
+               "  int x;\n"
+               "  };\n"
+               "  }",
+               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"
+               "    }\n"
+               "  while (false)\n"
+               "  }\n",
+               WhitesmithsBraceStyle);
+
+  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",
+               WhitesmithsBraceStyle);
+
+  verifyFormat("enum X\n"
+               "  {\n"
+               "  Y = 0,\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() { int i = 5; }\n"
+               "#ifdef _DEBUG\n"
+               "void bar() {}\n"
+               "#else\n"
+               "void bar() { foobar(); }\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);
+
+  verifyFormat("void f(Attribute Attribute)\n"
+               "  {\n"
+               "  auto ItFind = std::find_if(randomcontainerlongname.cbegin(),\n"
+               "                             randomcontainerlongname.cend(),\n"
+               "                             [&Attribute](const Attribute &Element) -> bool\n"
+         "                               {\n"
+         "                               return true;\n"
+         "                               });\n"
+               "  }",
+               WhitesmithsBraceStyle);
+
+  WhitesmithsBraceStyle.ColumnLimit = 19;
+  verifyFormat("void f() { int i; }", WhitesmithsBraceStyle);
+  WhitesmithsBraceStyle.ColumnLimit = 18;
+  verifyFormat("void f()\n"
+               "  {\n"
+               "  int i;\n"
+               "  }",
+               WhitesmithsBraceStyle);
+  WhitesmithsBraceStyle.ColumnLimit = 80;
+
+  FormatStyle BreakBeforeBraceShortIfs = WhitesmithsBraceStyle;
+  BreakBeforeBraceShortIfs.AllowShortIfStatementsOnASingleLine = true;
+  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;

                "  Y = 0,\n"
                "}\n",
                GNUBraceStyle);
+  verifyFormat("enum X\n"
+               "{\n"
+               "  Y = 0\n"
+               "};\n",
+               GNUBraceStyle);
 
   verifyFormat("@interface BSApplicationController ()\n"
                "{\n"

               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);
 
   Style.NamespaceIndentation = FormatStyle::NI_All;