[Clang-Format] Add AlwaysBreakBeforeElse and AlwaysBreakBeforeCatch Style to avoid cuddled else/catch

[MyDeveloperDay]

Summary

Implementation of Jarkko Hietaniemi original AlwaysBreakBeforeElse and AlwaysBreakBeforeCatch to avoid cuddled braces on an "else/catch" statement.

WebKit Style + Linux BreakBeforeBraces style gives cuddled else/catch brackets (e.g. "} else {") , this patch adds an AlwaysBreakBeforeElse and AlwaysBreakBeforeCatch style to allow required brace style such that the else/catch is placed on a newline after the closing brace of the if/try

Patch by: Jarkko Hietaniemi

[MyDeveloperDay]

As this patch has not been reviewed yet, and in using the tool in my own environment I noticed the same is true for catch statments, I want to see
try {
}
catch (exception &e) {
}

not

try {
} catch (exception &e) {
}

[MyDeveloperDay]

Handle subsequent catch statements

[djasper]

Please upload a patch with full file context.

docs/ClangFormatStyleOptions.rst

247

Hm, I think these should be grouped in some way and interact with BreakBeforeBraces.

I theory, I'd like to have a lot of different flags and make BreakBeforeBraces select specific presents for them. However, maybe we can put all the brace breaking options together in some nice way (don't have a very good idea yet).

Not saying that you need to do all of this, but pulling these two options out from the other BreakBeforeBraces options seems a little undesirable. They essentially also just define what we do around braces.

[MyDeveloperDay]

full file diff

[MyDeveloperDay]

docs/ClangFormatStyleOptions.rst

247

Well I agree, ideally the BreakBeforeBraces styles should be implemented by setting the finite control on each element

Then code like this

if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||

Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup ||
Style.AlwaysBreakBeforeCatch) {
 ...
}

could become

if (Style.AlwaysBreakBeforeCatch) {

...

}

This would simply require some code in the initializer that says

if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||

Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup)

{

Style.AlwaysBreakBeforeCatch=true;

}

if (Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup)
{

Style.AlwaysBreakBeforeElse=true;

}

This would allow those of us not using one of the 5 chosen styles to build our teams style guide via the individual capabilities

Removing all the BS_Stroustrup/Allman etc.. from the UnwrappedLineParser will greatly improve its readability

[djasper]

The question is how all the other options should be called and how we can
group them nicely so that we don't flood the options configuration page too
much. Maybe it's time for nesting in the configuration class?

[djasper]

If that's ok for you, I'd like to try and restructure the existing options first. I'll try to complete that this week. Will update this review once I am done.

[MyDeveloperDay]

@djasper I'm more than happy with that, just ping me if you need me the rework the change afterwards to fit

[djasper]

I think r248802 should make this mostly obsolete, but feel free to add the tests :-).

[MyDeveloperDay]

I don't have permission to commit

This looks good though.

[djasper]

What do you mean by "this looks good"?

docs/ClangFormatStyleOptions.rst

**AlwaysBreakBeforeMultilineStrings** (``bool``)
   in a file look more consistent. Thus, it will only take effect if wrapping
   the string at that point leads to it being indented
   ``ContinuationIndentWidth`` spaces from the start of the line.
 
 **AlwaysBreakTemplateDeclarations** (``bool``)
   If ``true``, always break after the ``template<...>`` of a
   template declaration.
 
+**AlwaysBreakBeforeElse** (``bool``)
+  If ``true``, always break before ``else``.
+
+**AlwaysBreakBeforeCatch** (``bool``)
+  If ``true``, always break before ``catch``.
+
 **BinPackArguments** (``bool``)
   If ``false``, a function call's arguments will either be all on the
   same line or will have one line each.
 
 **BinPackParameters** (``bool``)
   If ``false``, a function declaration's or function definition's
   parameters will either all be on the same line or will have one line each.
 

include/clang/Format/Format.h

struct FormatStyle {
   /// the string at that point leads to it being indented
   /// \c ContinuationIndentWidth spaces from the start of the line.
   bool AlwaysBreakBeforeMultilineStrings;
 
   /// \brief If \c true, always break after the <tt>template<...></tt> of a
   /// template declaration.
   bool AlwaysBreakTemplateDeclarations;
 
+  /// \brief If \c true, always break before 'else'.
+  bool AlwaysBreakBeforeElse;
+
+  /// \brief If \c true, always break before 'catch'.
+  bool AlwaysBreakBeforeCatch;
+
   /// \brief If \c false, a function call's arguments will either be all on the
   /// same line or will have one line each.
   bool BinPackArguments;
 
   /// \brief If \c false, a function declaration's or function definition's
   /// parameters will either all be on the same line or will have one line each.
   bool BinPackParameters;
 
return AccessModifierOffset == R.AccessModifierOffset &&
                R.AllowShortIfStatementsOnASingleLine &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
            AlwaysBreakAfterDefinitionReturnType ==
                R.AlwaysBreakAfterDefinitionReturnType &&
            AlwaysBreakBeforeMultilineStrings ==
                R.AlwaysBreakBeforeMultilineStrings &&
            AlwaysBreakTemplateDeclarations ==
                R.AlwaysBreakTemplateDeclarations &&
+           AlwaysBreakBeforeElse == R.AlwaysBreakBeforeElse &&
+           AlwaysBreakBeforeCatch == R.AlwaysBreakBeforeCatch &&
            BinPackArguments == R.BinPackArguments &&
            BinPackParameters == R.BinPackParameters &&
            BreakBeforeBinaryOperators == R.BreakBeforeBinaryOperators &&
            BreakBeforeBraces == R.BreakBeforeBraces &&
            BreakBeforeTernaryOperators == R.BreakBeforeTernaryOperators &&
            BreakConstructorInitializersBeforeComma ==
                R.BreakConstructorInitializersBeforeComma &&
-           ColumnLimit == R.ColumnLimit &&
-           CommentPragmas == R.CommentPragmas &&
+           ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
            ConstructorInitializerAllOnOneLineOrOnePerLine ==
                R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&
            ContinuationIndentWidth == R.ContinuationIndentWidth &&
            Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
return AccessModifierOffset == R.AccessModifierOffset &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
-           Standard == R.Standard &&
-           TabWidth == R.TabWidth &&
+           Standard == R.Standard && TabWidth == R.TabWidth &&
            UseTab == R.UseTab;
   }
 };
 
 /// \brief Returns a format style complying with the LLVM coding standards:
 /// http://llvm.org/docs/CodingStandards.html.
 FormatStyle getLLVMStyle();
 

lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",
                    Style.AllowShortIfStatementsOnASingleLine);
     IO.mapOptional("AllowShortLoopsOnASingleLine",
                    Style.AllowShortLoopsOnASingleLine);
     IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
                    Style.AlwaysBreakAfterDefinitionReturnType);
     IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
                    Style.AlwaysBreakBeforeMultilineStrings);
+    IO.mapOptional("AlwaysBreakBeforeElse", Style.AlwaysBreakBeforeElse);
+    IO.mapOptional("AlwaysBreakBeforeCatch", Style.AlwaysBreakBeforeCatch);
     IO.mapOptional("AlwaysBreakTemplateDeclarations",
                    Style.AlwaysBreakTemplateDeclarations);
     IO.mapOptional("BinPackArguments", Style.BinPackArguments);
     IO.mapOptional("BinPackParameters", Style.BinPackParameters);
     IO.mapOptional("BreakBeforeBinaryOperators",
                    Style.BreakBeforeBinaryOperators);
     IO.mapOptional("BreakBeforeBraces", Style.BreakBeforeBraces);
     IO.mapOptional("BreakBeforeTernaryOperators",
FormatStyle getLLVMStyle() {
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
+  LLVMStyle.AlwaysBreakBeforeElse = false;
+  LLVMStyle.AlwaysBreakBeforeCatch = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BreakConstructorInitializersBeforeComma = false;
   LLVMStyle.ColumnLimit = 80;
FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
   FormatStyle GoogleStyle = getLLVMStyle();
   GoogleStyle.Language = Language;
 
   GoogleStyle.AccessModifierOffset = -1;
   GoogleStyle.AlignEscapedNewlinesLeft = true;
   GoogleStyle.AllowShortIfStatementsOnASingleLine = true;
   GoogleStyle.AllowShortLoopsOnASingleLine = true;
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
+  GoogleStyle.AlwaysBreakBeforeElse = false;
+  GoogleStyle.AlwaysBreakBeforeCatch = false;
   GoogleStyle.AlwaysBreakTemplateDeclarations = true;
   GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCSpaceAfterProperty = false;
   GoogleStyle.ObjCSpaceBeforeProtocolList = false;
   GoogleStyle.PointerAlignment = FormatStyle::PAS_Left;

lib/Format/UnwrappedLineParser.cpp

if (FormatTok->Tok.is(tok::l_brace)) {
     }
   } else {
     addUnwrappedLine();
     ++Line->Level;
     parseStructuralElement();
     --Line->Level;
   }
   if (FormatTok->Tok.is(tok::kw_else)) {
-    if (Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup)
+    if (Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup ||
+        Style.AlwaysBreakBeforeElse) {
       addUnwrappedLine();
+    }
+
     nextToken();
     if (FormatTok->Tok.is(tok::l_brace)) {
       CompoundStatementIndenter Indenter(this, Style, Line->Level);
       parseBlock(/*MustBeDeclaration=*/false);
       addUnwrappedLine();
     } else if (FormatTok->Tok.is(tok::kw_if)) {
       parseIfThenElse();
     } else {
void UnwrappedLineParser::parseTryCatch() {
   if (Style.Language == FormatStyle::LK_Java && FormatTok->is(tok::l_paren)) {
     parseParens();
   }
   if (FormatTok->is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
-        Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
+        Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup ||
+        Style.AlwaysBreakBeforeCatch) {
       addUnwrappedLine();
     } else {
       NeedsUnwrappedLine = true;
     }
   } else if (!FormatTok->is(tok::kw_catch)) {
     // The C++ standard requires a compound-statement after a try.
     // If there's none, we try to assume there's a structuralElement
     // and try to continue.
while (FormatTok->isNot(tok::l_brace)) {
         return;
       nextToken();
     }
     NeedsUnwrappedLine = false;
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
         Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
-        Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
+        Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup ||
+        Style.AlwaysBreakBeforeCatch) {
       addUnwrappedLine();
     } else {
       NeedsUnwrappedLine = true;
     }
   }
   if (NeedsUnwrappedLine) {
     addUnwrappedLine();
   }

tools/CMakeLists.txt

if(CLANG_ENABLE_STATIC_ANALYZER)
   add_subdirectory(clang-check)
 endif()
 
 # We support checking out the clang-tools-extra repository into the 'extra'
 # subdirectory. It contains tools developed as part of the Clang/LLVM project
 # on top of the Clang tooling platform. We keep them in a separate repository
 # to keep the primary Clang repository small and focused.
 # It also may be included by LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR.
-add_llvm_external_project(clang-tools-extra extra)
+#add_llvm_external_project(clang-tools-extra extra)
+add_subdirectory(extra)

unittests/Format/FormatTest.cpp

verifyFormat("struct S {\n"
                "  } Value;\n"
                "  class C {\n"
                "    MyFavoriteType Value;\n"
                "  } Class;\n"
                "};\n",
                WebKitBraceStyle);
 }
 
+TEST_F(FormatTest, AlwaysBreakBeforeElse) {
+  FormatStyle BeforeElse = getLLVMStyle();
+  BeforeElse.AlwaysBreakBeforeElse = true;
+  verifyFormat("void foo() {\n"
+               "  if (a) {\n"
+               "    a();\n"
+               "  }\n"
+               "  else {\n"
+               "    b();\n"
+               "  }\n"
+               "}\n",
+               BeforeElse);
+
+  verifyFormat("void foo() {\n"
+               "  if (a) {\n"
+               "    a();\n"
+               "  }\n"
+               "  else if (b) {\n"
+               "    b();\n"
+               "  }\n"
+               "  else {\n"
+               "    b();\n"
+               "  }\n"
+               "}\n",
+               BeforeElse);
+}
+
+TEST_F(FormatTest, AlwaysBreakBeforeCatch) {
+  FormatStyle BeforeCatch = getLLVMStyle();
+  BeforeCatch.AlwaysBreakBeforeCatch = true;
+  verifyFormat("void foo() {\n"
+               "  try {\n"
+               "    a();\n"
+               "  }\n"
+               "  catch (exception &) {\n"
+               "    b();\n"
+               "  }\n"
+               "  catch (exception &) {\n"
+               "    b();\n"
+               "  }\n"
+               "}\n",
+               BeforeCatch);
+}
+
 TEST_F(FormatTest, CatchExceptionReferenceBinding) {
   verifyFormat("void f() {\n"
                "  try {\n"
                "  } catch (const Exception &e) {\n"
                "  }\n"
                "}\n",
                getLLVMStyle());
 }