[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pavlo Shkrabliuk via Phabricator via cfe-commits]

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();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71939.235414.patch
Type: text/x-patch
Size: 3483 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191227/2b19613c/attachment.bin>