pastey created this revision.
pastey added a reviewer: MyDeveloperDay.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Hi,
Found a bug introduced with BraceWrappingFlags AfterControlStatement MultiLine.
This feature conflicts with the existing BeforeCatch and BeforeElse flags.
For example, our team uses BeforeElse.
if (foo ||
bar) {
doSomething();
}
else {
doSomethingElse();
}
If we enable MultiLine (which we'd really love to do) we expect it to work like
this:
if (foo ||
bar)
{
doSomething();
}
else {
doSomethingElse();
}
What we actually get is:
if (foo ||
bar)
{
doSomething();
}
else
{
doSomethingElse();
}
I added two tests that show this issue. One for if/else and one for try/catch
(which is affected in the same way as if/else).
One more test with ColumnLimit set to 40 is to check that a suggested fix
doesn't break existing cases. This test is the copy of an existing test from
the same case, but it removes line wrap from the stage.
I suggest the fix, but I'm new to this code, so maybe you could suggest
something better.
As far as I understood, the problem is caused by the following code:
CompoundStatementIndenter(UnwrappedLineParser *Parser,
const FormatStyle &Style, unsigned &LineLevel)
: CompoundStatementIndenter(Parser, LineLevel,
Style.BraceWrapping.AfterControlStatement,
// <----- here
Style.BraceWrapping.IndentBraces) {}
CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel,
bool WrapBrace, bool IndentBrace)
: LineLevel(LineLevel), OldLineLevel(LineLevel) {
if (WrapBrace)
// <----- and here
Parser->addUnwrappedLine();
if (IndentBrace)
++LineLevel;
}
Style.BraceWrapping.AfterControlStatement used to be boolean, now it's an enum.
MultiLine and Always both turn into true, thus MultiLine leads to a call of
addUnwrappedLine().
I tried to place Style.BraceWrapping.AfterControlStatement ==
FormatStyle::BWACS_Always right in the CompoundStatementIndenter constructor,
but that breaks MultiLine.
As I said, I'm new to this code, so maybe my fix is not at the right place, so
I would be glad if we find a better fix.
Repository:
rC Clang
https://reviews.llvm.org/D71939
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
" baz();\n"
"}",
format("try{foo();}catch(Exception&bar){baz();}", Style));
+ Style.ColumnLimit =
+ 40; // to concentrate at brace wrapping, not line wrap due to column
limit
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "} catch (Exception &bar) {\n"
+ " baz();\n"
+ "}",
+ format("try{foo();}catch(Exception&bar){baz();}", Style));
+ Style.ColumnLimit =
+ 20; // to concentrate at brace wrapping, not line wrap due to column
limit
+
+ Style.BraceWrapping.BeforeElse = true;
+ EXPECT_EQ(
+ "if (foo) {\n"
+ " bar();\n"
+ "}\n"
+ "else if (baz ||\n"
+ " quux)\n"
+ "{\n"
+ " foobar();\n"
+ "}\n"
+ "else {\n"
+ " barbaz();\n"
+ "}",
+ format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+ Style.BraceWrapping.BeforeCatch = true;
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "}\n"
+ "catch (...) {\n"
+ " baz();\n"
+ "}",
+ format("try{foo();}catch(...){baz();}", Style));
}
//===----------------------------------------------------------------------===//
@@ -14881,7 +14916,7 @@
verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
}
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
FormatStyle Spaces = getLLVMStyle();
Spaces.SpacesInConditionalStatement = true;
verifyFormat("for ( int i = 0; i; i++ )\n continue;", Spaces);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1785,7 +1785,11 @@
if (FormatTok->Tok.is(tok::kw_else)) {
nextToken();
if (FormatTok->Tok.is(tok::l_brace)) {
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
addUnwrappedLine();
} else if (FormatTok->Tok.is(tok::kw_if)) {
@@ -1823,7 +1827,10 @@
parseParens();
}
if (FormatTok->is(tok::l_brace)) {
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (Style.BraceWrapping.BeforeCatch) {
addUnwrappedLine();
@@ -1861,7 +1868,10 @@
nextToken();
}
NeedsUnwrappedLine = false;
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (Style.BraceWrapping.BeforeCatch)
addUnwrappedLine();
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
" baz();\n"
"}",
format("try{foo();}catch(Exception&bar){baz();}", Style));
+ Style.ColumnLimit =
+ 40; // to concentrate at brace wrapping, not line wrap due to column limit
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "} catch (Exception &bar) {\n"
+ " baz();\n"
+ "}",
+ format("try{foo();}catch(Exception&bar){baz();}", Style));
+ Style.ColumnLimit =
+ 20; // to concentrate at brace wrapping, not line wrap due to column limit
+
+ Style.BraceWrapping.BeforeElse = true;
+ EXPECT_EQ(
+ "if (foo) {\n"
+ " bar();\n"
+ "}\n"
+ "else if (baz ||\n"
+ " quux)\n"
+ "{\n"
+ " foobar();\n"
+ "}\n"
+ "else {\n"
+ " barbaz();\n"
+ "}",
+ format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+ Style.BraceWrapping.BeforeCatch = true;
+ EXPECT_EQ("try {\n"
+ " foo();\n"
+ "}\n"
+ "catch (...) {\n"
+ " baz();\n"
+ "}",
+ format("try{foo();}catch(...){baz();}", Style));
}
//===----------------------------------------------------------------------===//
@@ -14881,7 +14916,7 @@
verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
}
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
FormatStyle Spaces = getLLVMStyle();
Spaces.SpacesInConditionalStatement = true;
verifyFormat("for ( int i = 0; i; i++ )\n continue;", Spaces);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1785,7 +1785,11 @@
if (FormatTok->Tok.is(tok::kw_else)) {
nextToken();
if (FormatTok->Tok.is(tok::l_brace)) {
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement ==
+ FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
addUnwrappedLine();
} else if (FormatTok->Tok.is(tok::kw_if)) {
@@ -1823,7 +1827,10 @@
parseParens();
}
if (FormatTok->is(tok::l_brace)) {
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (Style.BraceWrapping.BeforeCatch) {
addUnwrappedLine();
@@ -1861,7 +1868,10 @@
nextToken();
}
NeedsUnwrappedLine = false;
- CompoundStatementIndenter Indenter(this, Style, Line->Level);
+ CompoundStatementIndenter Indenter(
+ this, Line->Level,
+ Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (Style.BraceWrapping.BeforeCatch)
addUnwrappedLine();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
