[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

@MyDeveloperDay I don't have commit rights, could you land this change for me 
please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska updated this revision to Diff 253366.
Bouska added a comment.

Do not touch current tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -606,6 +606,12 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  ff();\n"
"}",
@@ -681,6 +687,13 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+   "{\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
"  ff();\n"
@@ -745,7 +758,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -349,21 +349,6 @@
  ? 1
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -606,6 +606,12 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  ff();\n"
"}",
@@ -681,6 +687,13 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+   "{\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
"  

[PATCH] D73354: clang-format: insert trailing commas into containers.

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

I have an issue with this change. Currently (at least for C++), the presence of 
a trailing comma is used as a formatting hint to put all the element in one 
line or one per line as below:

  enum test1 = {ONE, TWO, THREE};
  
  enum test2 = {
  ONE,
  TWO,
  THREE,
  };

As your change come after the formatting, clang-format won't be idempotent 
anymore. You are going to have the following if the container is bin packed (I 
did not actually test this):

  First run:  
  enum test1 = {
  ONE, TWO,
  THREE,};
  
  Second run:
  enum test1 = {
  ONE,
  TWO,
  THREE,
  };

So I definitely think this change should be done before formatting and not 
after.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73354: clang-format: insert trailing commas into containers.

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73354/new/

https://reviews.llvm.org/D73354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska marked an inline comment as done.
Bouska added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:662
"{\n"
-   "  ff();\n"
"}",

MyDeveloperDay wrote:
> I'm just not comfortable with changing tests, code can still be written in 
> the way it was before, and I cannot tell what it would now be, I believe 
> tests should be additive
Ok, I'll keep the original test and add two new ones.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska marked an inline comment as done.
Bouska added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:344
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks

MyDeveloperDay wrote:
> I just can't understand how we are free to just remove a chunk of code. Why 
> is this not being some sort of 
> 
> 
> ```
> if (Style.AllowShortBlocksOnASingleLine)
> ```
> 
> You are just not giving a convincing argument that this is a safe change 
> without us having to go in a debug the original problem ourselves.
This chunk of code only provides a bogus formatting and nothing else. So in 
order to fix the bug, I need to remove the whole block of code.

I wasn't clear on why this chunk of code created the bug, so let me explained. 
Let's take an example:
```
if (true)
{
doStuff();
}
```
with //Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always// 
and //Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never//.

When formatting the first line, the control statement @ line 308 is going to 
fail (because //Style.BraceWrapping.AfterControlStatement != 
FormatStyle::BWACS_Always//) but the control statement @ line 322 is going to 
pass (because the second line is a left brace and the first token of the line 
is a //if//). As //Style.BraceWrapping.AfterControlStatement == 
FormatStyle::BWACS_Always//, //tryMergeSimpleBlock()// is going to handle the 
merging. There is two possible outcomes : either the block is simple and not 
too long and it can be merge in one line, or it is not and the whole block 
stays as it is. That line is then considered formated (due to the //return//).

When formatting the second line, the control statement @ line 332 (because the 
first and only token of the line is a left brace and the first token of the 
first line is a //if//) is going to pass and 
//Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never//, so we are 
going to redo //tryMergeSimpleBlock()// on the first line which failed the 
first time! So this is the part I understand, we are doing someting we already 
did. The part I don't understand is that this time //tryMergeSimpleBlock()// 
managed to merge the line (probably because of some changes on the annoted 
first line). As the merging worked, //MergedLines > 0// @ line 340 is true so 
//MergedLines// is decremented (as explained by the comment) and we end up with 
the block merged but without the control statement:
 ```
if (true)
{ doStuff(); }
```
Which is not the expected formating.

I think there might be another bug hidden somewhere in 
//tryMergeSimpleBlock()//.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Ping @MyDeveloperDay @klimek


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska updated this revision to Diff 236188.
Bouska added a comment.

Updated unittest to check under the column limit and over the column limit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -582,8 +582,10 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -657,9 +659,11 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
@@ -721,7 +725,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -327,21 +327,6 @@
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -582,8 +582,10 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -657,9 +659,11 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   

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

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

I'm not sure creating a special case for else/catch is the best idea. I would 
seem better to change the control statement @ line 309 to add it the case where 
we break before else or catch (add a new || test in the middle condition). I'm 
really new to this code too, so I don't know if it's better to check that 
previous line as a right curly brace and that BeforeElse/Catch is true and that 
first token of the line is else or catch or if just checking that the first 
token of the line is else or catch (or a mix in between). I'll let a more 
senior reviewer advise on this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71939/new/

https://reviews.llvm.org/D71939



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71659: [clang-format] Added new option to allow setting spaces before and after the operator

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Is there a reason why this option adds space before and after the tok::arrow 
token but not the tok::arrowstar token? That seems inconsistent to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71659/new/

https://reviews.llvm.org/D71659



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska requested changes to this revision.
Bouska added a comment.
This revision now requires changes to proceed.

So, you are trying to fix the issue at the wrong place. Contrary from what we 
should expect from a UnwrappedLine, BWACS_Multiline works by always wrapping 
the brace in UnwrappedLineParser, and afterwards in UnwrappedLineFormatter 
merging it back to the control statement if it's not in multi-line. The bug is 
on the control statement @ line 308 on UnwrappedLineFormatter.cpp 
.
 That's the block that merge the brace back to its control statement if it is 
not a multiline and it's executed only with a line with a else or a catch that 
begins with right curly brace.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71939/new/

https://reviews.llvm.org/D71939



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

Your code still breaks MultiLine

  [ RUN  ] FormatTest.MultiLineControlStatements
  /home/pablomg/dev/llvm-project/clang/unittests/Format/FormatTest.cpp:1562: 
Failure
Expected: "try {\n" "  foo();\n" "} catch (\n" "Exception )\n" 
"{\n" "  baz();\n" "}"
Which is: "try {\n  foo();\n} catch (\nException )\n{\n  
baz();\n}"
  To be equal to: format("try{foo();}catch(Exception){baz();}", Style)
Which is: "try {\n  foo();\n} catch (Exception\n ) {\n  
baz();\n}"
  With diff:
  @@ -1,7 +1,6 @@
   try {
 foo();
  -} catch (
  -Exception )
  -{
  +} catch (Exception
  + ) {
 baz();
   }
  
  [  FAILED  ] FormatTest.MultiLineControlStatements (28 ms)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71939/new/

https://reviews.llvm.org/D71939



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska updated this revision to Diff 235435.
Bouska added a comment.

Set line length to column limit + 1 (41) in tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -583,7 +583,7 @@
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -659,7 +659,7 @@
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
@@ -721,7 +721,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -327,21 +327,6 @@
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -583,7 +583,7 @@
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -659,7 +659,7 @@
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
@@ -721,7 +721,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -327,21 +327,6 @@
  ? tryMergeSimpleBlock(I, E, Limit)
 

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:586
   verifyFormat("if (true) {\n"
-   "  ff();\n"
"}",

MyDeveloperDay wrote:
> any reason you are changing existing tests?
According to the change that added this test 
(https://reviews.llvm.org/D37140?id=113074), this test was made to check that 
"the wrapping is forced by exceeding the column limit". This should be check at 
the limit condition when the control statement + the block exceed the column 
limit by a few characters. With the current test, the length of the block 
statement alone is sufficient to force the wrapping, that's why with the 
current code this test has the expected wrapping.

Even my change is incorrect, with the control statement, the curly braces, the 
spaces and the statement, it is way above the column limit of 40. I'm going to 
fix that. Alternatively, I just could add another test with the correct length. 
  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71512/new/

https://reviews.llvm.org/D71512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska created this revision.
Bouska added reviewers: djasper, klimek, krasimir.
Bouska added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch fixes bug #44192

When clang-format is run with option AllowShortBlocksOnASingleLine, it is 
expected to either succeed in putting the short block with its control 
statement on a single line or fail and leave the block as is. When brace 
wrapping after control statement is activated, if the block + the control 
statement length is superior to column limit but the block alone is not, 
clang-format puts the block in two lines: one for the control statement and one 
for the block. This patch removes this unexpected behaviour. Current unittests 
are updated to check for this behaviour.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71512

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -583,7 +583,7 @@
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ff();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -659,7 +659,7 @@
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ff();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
@@ -721,7 +721,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -327,21 +327,6 @@
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -583,7 +583,7 @@
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
-   "  ff();\n"
+   "  ff();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -659,7 +659,7 @@
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
-   "  ff();\n"
+   "  ff();\n"
"}",
AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
@@ -721,7 +721,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if