mitchell-stellar updated this revision to Diff 222835.
mitchell-stellar edited the summary of this revision.
mitchell-stellar added a comment.
Thanks for the review. I have added documentation updates.
I do not have a public style guide to reference. My company just switched to
auto-clang-formatting all of our code, and this patch addresses one of our
biggest pain points: readability. When the opening brace of a multi-line
control statement is not wrapped, we simply cannot tell at first glance where
the control statement ends and where the body begins due to our indentation
settings.
I think the fact that you yourself see the benefit of this change speaks to its
viability.
I cannot think of any other brace wrapping rules that need this level of
control. Our code base consists of millions of lines of code, so I'm pretty
sure we would have found another instance.
I would be willing to support this patch and further revisions if this gets
into master, as our company will depend on this feature.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68296/new/
https://reviews.llvm.org/D68296
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestObjC.cpp
Index: clang/unittests/Format/FormatTestObjC.cpp
===================================================================
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -207,7 +207,7 @@
" f();\n"
"}\n");
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
verifyFormat("@autoreleasepool\n"
"{\n"
" f();\n"
@@ -237,7 +237,7 @@
" f();\n"
"}\n");
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
verifyFormat("@synchronized(self)\n"
"{\n"
" f();\n"
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -644,7 +644,8 @@
AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
FormatStyle::SIS_WithoutElse;
AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
- AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
+ AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement =
+ FormatStyle::BWACS_Always;
verifyFormat("if (true) {}", AllowSimpleBracedStatements);
verifyFormat("if constexpr (true) {}", AllowSimpleBracedStatements);
@@ -1168,7 +1169,7 @@
Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.AfterCaseLabel = true;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
EXPECT_EQ("switch (n)\n"
"{\n"
" case 0:\n"
@@ -1370,7 +1371,7 @@
Style.AllowShortCaseLabelsOnASingleLine = true;
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.AfterCaseLabel = true;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
EXPECT_EQ("switch (n)\n"
"{\n"
" case 0:\n"
@@ -1441,6 +1442,126 @@
"}");
}
+TEST_F(FormatTest, MultiLineControlStatements) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakBeforeBraces = FormatStyle::BraceBreakingStyle::BS_Custom;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_OnlyMultiLine;
+ Style.ColumnLimit = 20;
+ // Short lines should keep opening brace on same line.
+ EXPECT_EQ("if (foo) {\n"
+ " bar();\n"
+ "}",
+ format("if(foo){bar();}", Style));
+ EXPECT_EQ("if (foo) {\n"
+ " bar();\n"
+ "} else {\n"
+ " baz();\n"
+ "}",
+ format("if(foo){bar();}else{baz();}", Style));
+ EXPECT_EQ("if (foo && bar) {\n"
+ " baz();\n"
+ "}",
+ format("if(foo&&bar){baz();}", Style));
+ EXPECT_EQ("if (foo) {\n"
+ " bar();\n"
+ "} else if (baz) {\n"
+ " quux();\n"
+ "}",
+ format("if(foo){bar();}else if(baz){quux();}", Style));
+ EXPECT_EQ(
+ "if (foo) {\n"
+ " bar();\n"
+ "} else if (baz) {\n"
+ " quux();\n"
+ "} else {\n"
+ " foobar();\n"
+ "}",
+ format("if(foo){bar();}else if(baz){quux();}else{foobar();}", Style));
+ EXPECT_EQ("for (;;) {\n"
+ " foo();\n"
+ "}",
+ format("for(;;){foo();}"));
+ EXPECT_EQ("while (1) {\n"
+ " foo();\n"
+ "}",
+ format("while(1){foo();}", Style));
+ EXPECT_EQ("switch (foo) {\n"
+ "case bar:\n"
+ " return;\n"
+ "}",
+ format("switch(foo){case bar:return;}", Style));
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "} catch (...) {\n"
+ " bar();\n"
+ "}",
+ format("try{foo();}catch(...){bar();}", Style));
+ // Long lines should put opening brace on new line.
+ EXPECT_EQ("if (foo && bar &&\n"
+ " baz)\n"
+ "{\n"
+ " quux();\n"
+ "}",
+ format("if(foo&&bar&&baz){quux();}", Style));
+ EXPECT_EQ("if (foo && bar &&\n"
+ " baz)\n"
+ "{\n"
+ " quux();\n"
+ "}",
+ format("if (foo && bar &&\n"
+ " baz) {\n"
+ " quux();\n"
+ "}",
+ Style));
+ EXPECT_EQ("if (foo) {\n"
+ " bar();\n"
+ "} else if (baz ||\n"
+ " quux)\n"
+ "{\n"
+ " foobar();\n"
+ "}",
+ format("if(foo){bar();}else if(baz||quux){foobar();}", Style));
+ EXPECT_EQ(
+ "if (foo) {\n"
+ " bar();\n"
+ "} else if (baz ||\n"
+ " quux)\n"
+ "{\n"
+ " foobar();\n"
+ "} else {\n"
+ " barbaz();\n"
+ "}",
+ format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+ EXPECT_EQ("for (int i = 0;\n"
+ " i < 10; ++i)\n"
+ "{\n"
+ " foo();\n"
+ "}",
+ format("for(int i=0;i<10;++i){foo();}", Style));
+ EXPECT_EQ("while (foo || bar ||\n"
+ " baz)\n"
+ "{\n"
+ " quux();\n"
+ "}",
+ format("while(foo||bar||baz){quux();}", Style));
+ EXPECT_EQ("switch (\n"
+ " foo = barbaz)\n"
+ "{\n"
+ "case quux:\n"
+ " return;\n"
+ "}",
+ format("switch(foo=barbaz){case quux:return;}", Style));
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "} catch (\n"
+ " Exception &bar)\n"
+ "{\n"
+ " baz();\n"
+ "}",
+ format("try{foo();}catch(Exception&bar){baz();}", Style));
+}
+
//===----------------------------------------------------------------------===//
// Tests for classes, namespaces, etc.
//===----------------------------------------------------------------------===//
@@ -2940,7 +3061,7 @@
"};"));
FormatStyle Style = getLLVMStyle();
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
Style.BraceWrapping.AfterFunction = true;
EXPECT_EQ("void f()\n"
"try\n"
@@ -12248,7 +12369,6 @@
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
- CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
@@ -12449,6 +12569,25 @@
CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
FormatStyle::BS_Custom);
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
+ CHECK_PARSE("BraceWrapping:\n"
+ " AfterControlStatement: OnlyMultiLine",
+ BraceWrapping.AfterControlStatement,
+ FormatStyle::BWACS_OnlyMultiLine);
+ CHECK_PARSE("BraceWrapping:\n"
+ " AfterControlStatement: Always",
+ BraceWrapping.AfterControlStatement, FormatStyle::BWACS_Always);
+ CHECK_PARSE("BraceWrapping:\n"
+ " AfterControlStatement: Never",
+ BraceWrapping.AfterControlStatement, FormatStyle::BWACS_Never);
+ // For backward compatibility:
+ CHECK_PARSE("BraceWrapping:\n"
+ " AfterControlStatement: true",
+ BraceWrapping.AfterControlStatement, FormatStyle::BWACS_Always);
+ CHECK_PARSE("BraceWrapping:\n"
+ " AfterControlStatement: false",
+ BraceWrapping.AfterControlStatement, FormatStyle::BWACS_Never);
+
Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
CHECK_PARSE("AlwaysBreakAfterReturnType: None", AlwaysBreakAfterReturnType,
FormatStyle::RTBS_None);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1167,7 +1167,8 @@
case tok::objc_autoreleasepool:
nextToken();
if (FormatTok->Tok.is(tok::l_brace)) {
- if (Style.BraceWrapping.AfterControlStatement)
+ if (Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always)
addUnwrappedLine();
parseBlock(/*MustBeDeclaration=*/false);
}
@@ -1179,7 +1180,8 @@
// Skip synchronization object
parseParens();
if (FormatTok->Tok.is(tok::l_brace)) {
- if (Style.BraceWrapping.AfterControlStatement)
+ if (Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always)
addUnwrappedLine();
parseBlock(/*MustBeDeclaration=*/false);
}
@@ -1989,7 +1991,8 @@
Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (FormatTok->Tok.is(tok::kw_break)) {
- if (Style.BraceWrapping.AfterControlStatement)
+ if (Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always)
addUnwrappedLine();
parseStructuralElement();
}
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -306,8 +306,24 @@
}
// Try to merge a control statement block with left brace wrapped
if (I[1]->First->is(tok::l_brace) &&
- TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
- return Style.BraceWrapping.AfterControlStatement
+ (TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
+ tok::kw_switch, tok::kw_try) ||
+ (TheLine->First->is(tok::r_brace) && TheLine->First->Next &&
+ TheLine->First->Next->isOneOf(tok::kw_else, tok::kw_catch))) &&
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_OnlyMultiLine) {
+ // If possible, merge the next line's wrapped left brace with the current
+ // line. Otherwise, leave it on the next line, as this is a multi-line
+ // control statement.
+ return (Style.ColumnLimit == 0 ||
+ TheLine->Last->TotalLength <= Style.ColumnLimit)
+ ? 1
+ : 0;
+ } else if (I[1]->First->is(tok::l_brace) &&
+ TheLine->First->isOneOf(tok::kw_if, tok::kw_while,
+ tok::kw_for)) {
+ return (Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always)
? tryMergeSimpleBlock(I, E, Limit)
: 0;
}
@@ -410,7 +426,8 @@
SmallVectorImpl<AnnotatedLine *>::const_iterator E, unsigned Limit) {
if (Limit == 0)
return 0;
- if (Style.BraceWrapping.AfterControlStatement &&
+ if (Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always &&
I[1]->First->is(tok::l_brace) &&
Style.AllowShortBlocksOnASingleLine == FormatStyle::SBS_Never)
return 0;
@@ -523,8 +540,9 @@
return 0;
if (!Style.AllowShortIfStatementsOnASingleLine &&
Line.startsWith(tok::kw_if) &&
- Style.BraceWrapping.AfterControlStatement && I + 2 != E &&
- !I[2]->First->is(tok::r_brace))
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always &&
+ I + 2 != E && !I[2]->First->is(tok::r_brace))
return 0;
if (!Style.AllowShortLoopsOnASingleLine &&
Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
@@ -533,8 +551,9 @@
return 0;
if (!Style.AllowShortLoopsOnASingleLine &&
Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
- Style.BraceWrapping.AfterControlStatement && I + 2 != E &&
- !I[2]->First->is(tok::r_brace))
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always &&
+ I + 2 != E && !I[2]->First->is(tok::r_brace))
return 0;
// FIXME: Consider an option to allow short exception handling clauses on
// a single line.
@@ -597,6 +616,17 @@
if (Tok->Next && Tok->Next->is(tok::kw_else))
return 0;
+ // Don't merge a trailing multi-line control statement block like:
+ // } else if (foo &&
+ // bar)
+ // { <-- current Line
+ // baz();
+ // }
+ if (Line.First == Line.Last &&
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_OnlyMultiLine)
+ return 0;
+
return 2;
}
} else if (I[1]->First->is(tok::l_brace)) {
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -172,6 +172,20 @@
}
};
+template <>
+struct ScalarEnumerationTraits<
+ FormatStyle::BraceWrappingAfterControlStatementStyle> {
+ static void
+ enumeration(IO &IO,
+ FormatStyle::BraceWrappingAfterControlStatementStyle &Value) {
+ IO.enumCase(Value, "false", FormatStyle::BWACS_Never);
+ IO.enumCase(Value, "true", FormatStyle::BWACS_Always);
+ IO.enumCase(Value, "Never", FormatStyle::BWACS_Never);
+ IO.enumCase(Value, "OnlyMultiLine", FormatStyle::BWACS_OnlyMultiLine);
+ IO.enumCase(Value, "Always", FormatStyle::BWACS_Always);
+ }
+};
+
template <>
struct ScalarEnumerationTraits<FormatStyle::BreakConstructorInitializersStyle> {
static void
@@ -620,9 +634,12 @@
if (Style.BreakBeforeBraces == FormatStyle::BS_Custom)
return Style;
FormatStyle Expanded = Style;
- Expanded.BraceWrapping = {false, false, false, false, false, false,
- false, false, false, false, false, false,
- false, true, true, true};
+ Expanded.BraceWrapping = {false, false, FormatStyle::BWACS_Never,
+ false, false, false,
+ false, false, false,
+ false, false, false,
+ false, true, true,
+ true};
switch (Style.BreakBeforeBraces) {
case FormatStyle::BS_Linux:
Expanded.BraceWrapping.AfterClass = true;
@@ -647,7 +664,7 @@
case FormatStyle::BS_Allman:
Expanded.BraceWrapping.AfterCaseLabel = true;
Expanded.BraceWrapping.AfterClass = true;
- Expanded.BraceWrapping.AfterControlStatement = true;
+ Expanded.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
Expanded.BraceWrapping.AfterEnum = true;
Expanded.BraceWrapping.AfterFunction = true;
Expanded.BraceWrapping.AfterNamespace = true;
@@ -661,7 +678,7 @@
case FormatStyle::BS_Whitesmiths:
Expanded.BraceWrapping.AfterCaseLabel = true;
Expanded.BraceWrapping.AfterClass = true;
- Expanded.BraceWrapping.AfterControlStatement = true;
+ Expanded.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
Expanded.BraceWrapping.AfterEnum = true;
Expanded.BraceWrapping.AfterFunction = true;
Expanded.BraceWrapping.AfterNamespace = true;
@@ -672,8 +689,12 @@
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};
+ Expanded.BraceWrapping = {true, true, FormatStyle::BWACS_Always,
+ true, true, true,
+ true, true, true,
+ true, true, true,
+ true, true, true,
+ true};
break;
case FormatStyle::BS_WebKit:
Expanded.BraceWrapping.AfterFunction = true;
@@ -713,9 +734,12 @@
LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
LLVMStyle.BreakBeforeTernaryOperators = true;
LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
- LLVMStyle.BraceWrapping = {false, false, false, false, false, false,
- false, false, false, false, false, false,
- false, true, true, true};
+ LLVMStyle.BraceWrapping = {false, false, FormatStyle::BWACS_Never,
+ false, false, false,
+ false, false, false,
+ false, false, false,
+ false, true, true,
+ true};
LLVMStyle.BreakAfterJavaFieldAnnotations = false;
LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
LLVMStyle.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
@@ -1058,7 +1082,7 @@
Style.UseTab = FormatStyle::UT_Never;
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.AfterClass = true;
- Style.BraceWrapping.AfterControlStatement = true;
+ Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
Style.BraceWrapping.AfterEnum = true;
Style.BraceWrapping.AfterFunction = true;
Style.BraceWrapping.AfterNamespace = true;
Index: clang/include/clang/Format/Format.h
===================================================================
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -782,6 +782,40 @@
/// The brace breaking style to use.
BraceBreakingStyle BreakBeforeBraces;
+ // Different ways to wrap braces after control statements.
+ enum BraceWrappingAfterControlStatementStyle {
+ /// Never wrap braces after a control statement.
+ /// \code
+ /// if (foo()) {
+ /// } else {
+ /// }
+ /// for (int i = 0; i < 10; ++i) {
+ /// }
+ /// \endcode
+ BWACS_Never,
+ /// Only wrap braces after a multi-line control statement.
+ /// \code
+ /// if (foo && bar &&
+ /// baz)
+ /// {
+ /// quux();
+ /// }
+ /// while (foo || bar) {
+ /// }
+ /// \endcode
+ BWACS_OnlyMultiLine,
+ /// Always wrap braces after a control statement.
+ /// \code
+ /// if (foo())
+ /// {
+ /// } else
+ /// {}
+ /// for (int i = 0; i < 10; ++i)
+ /// {}
+ /// \endcode
+ BWACS_Always
+ };
+
/// Precise control over the wrapping of braces.
/// \code
/// # Should be declared this way:
@@ -817,23 +851,7 @@
/// \endcode
bool AfterClass;
/// Wrap control statements (``if``/``for``/``while``/``switch``/..).
- /// \code
- /// true:
- /// if (foo())
- /// {
- /// } else
- /// {}
- /// for (int i = 0; i < 10; ++i)
- /// {}
- ///
- /// false:
- /// if (foo()) {
- /// } else {
- /// }
- /// for (int i = 0; i < 10; ++i) {
- /// }
- /// \endcode
- bool AfterControlStatement;
+ BraceWrappingAfterControlStatementStyle AfterControlStatement;
/// Wrap enum definitions.
/// \code
/// true:
Index: clang/docs/ClangFormatStyleOptions.rst
===================================================================
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -778,24 +778,46 @@
class foo
{};
- * ``bool AfterControlStatement`` Wrap control statements (``if``/``for``/``while``/``switch``/..).
+ * ``BraceWrappingAfterControlStatementStyle AfterControlStatement``
+ Wrap control statements (``if``/``for``/``while``/``switch``/..).
- .. code-block:: c++
+ Possible values:
- true:
- if (foo())
- {
- } else
- {}
- for (int i = 0; i < 10; ++i)
- {}
+ * ``BWACS_Never`` (in configuration: ``Never``)
+ Never wrap braces after a control statement.
- false:
- if (foo()) {
- } else {
- }
- for (int i = 0; i < 10; ++i) {
- }
+ .. code-block:: c++
+
+ if (foo()) {
+ } else {
+ }
+ for (int i = 0; i < 10; ++i) {
+ }
+
+ * ``BWACS_OnlyMultiLine`` (in configuration: ``OnlyMultiLine``)
+ Only wrap braces after a multi-line control statement.
+
+ .. code-block:: c++
+
+ if (foo && bar &&
+ baz)
+ {
+ quux();
+ }
+ while (foo || bar) {
+ }
+
+ * ``BWACS_Always`` (in configuration: ``Always``)
+ Always wrap braces after a control statement.
+
+ .. code-block:: c++
+
+ if (foo())
+ {
+ } else
+ {}
+ for (int i = 0; i < 10; ++i)
+ {}
* ``bool AfterEnum`` Wrap enum definitions.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
