[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG72e4f4a2a117: [clang-format] [PR47936] 
AfterControlStatement: MultiLine breaks… (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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
@@ -2860,6 +2860,19 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(...){baz();}", Style));
+
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  Style.ColumnLimit = 80;
+  verifyFormat("void shortfunction() { bar(); }", Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  verifyFormat("void shortfunction()\n"
+   "{\n"
+   "  bar();\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, BeforeWhile) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -676,7 +676,7 @@
 // { <-- current Line
 //   baz();
 // }
-if (Line.First == Line.Last &&
+if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace) &&
 Style.BraceWrapping.AfterControlStatement ==
 FormatStyle::BWACS_MultiLine)
   return 0;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2860,6 +2860,19 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(...){baz();}", Style));
+
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  Style.ColumnLimit = 80;
+  verifyFormat("void shortfunction() { bar(); }", Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  verifyFormat("void shortfunction()\n"
+   "{\n"
+   "  bar();\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, BeforeWhile) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -676,7 +676,7 @@
 // { <-- current Line
 //   baz();
 // }
-if (Line.First == Line.Last &&
+if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace) &&
 Style.BraceWrapping.AfterControlStatement ==
 FormatStyle::BWACS_MultiLine)
   return 0;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

The if/for/while case is way more involved and requires a change in a separate 
piece of code, I'm going to land this and we can handle 52598 separately


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I think the problem here is encapsulated by adding the following tests

   Style.BraceWrapping.AfterFunction = true;
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_AllIfsAndElse;
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
   Style.AllowShortLoopsOnASingleLine = true;
   Style.ColumnLimit = 80;
  
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   verifyFormat("void shortfunction() { bar(); }", Style);
   verifyFormat("if (x) { bar(); }", Style);
   verifyFormat("for (;;) { bar(); }", Style);
   verifyFormat("while (true) { bar(); }", Style);
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
   verifyFormat("void shortfunction() { bar(); }", Style);
   verifyFormat("if (x) { bar(); }", Style);
  >>> FAILS  ^^^
  
   verifyFormat("for (;;) { bar(); }", Style);
  >>> FAILS  ^^^
  
   verifyFormat("while (true) { bar(); }", Style);
  >>> FAILS  ^^^
  
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   Style.AllowShortLoopsOnASingleLine = false;
   verifyFormat("void shortfunction()\n"
"{\n"
"  foo();\n"
"}",
Style);
   verifyFormat("if (x) {\n"
"  foo();\n"
"}",
Style);
   verifyFormat("for (;;) {\n"
"  foo();\n"
"}",
Style);
   verifyFormat("while (true) {\n"
"  foo();\n"
"}",
Style);
  
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
   verifyFormat("void shortfunction()\n"
"{\n"
"  foo();\n"
"}",
Style);
   verifyFormat("if (x) {\n"
"  foo();\n"
"}",
Style);
   verifyFormat("for (;;) {\n"
"  foo();\n"
"}",
Style);
   verifyFormat("while (true) {\n"
"  foo();\n"
"}",
Style);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> @curdeius 
> Alright, I'll create a report https://bugs.llvm.org/show_bug.cgi?id=52598

That is related. But I'd rather handle it separately because the fix for the 
function and the fix for a while or if will need to be different.

It really comes down to this line..

  if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace)

in the if case and in the while case the `{` isn't labeled like it is for the 
function

  M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=0 Name=l_brace L=1 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='{'

To be honest this is something we should think about, if every `{` and `}` 
every `(`,`)` and `[`,`]` got given a Type labels this would be super good at 
disambiguating them

This is not something new, we've been doing it with the ')' of a cstyle cast 
for years `CastRParen`

  M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens=2/ 
FakeRParens=0 II=0x25a969b1660 Text='int'
  M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=5 PPK=2 
FakeLParens= FakeRParens=0 II=0x25a969b8768 Text='a'
  M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=7 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='='
  M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=l_paren L=9 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
  M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=int L=12 PPK=2 FakeLParens= 
FakeRParens=0 II=0x25a969b1660 Text='int'
  M=0 C=0 T=CastRParen S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=13 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text=')'

but in this case, the real problem here is that `Multiline`  just too lax, 
basically Multiline doesn't in my view seem to be doing what it expects

F20680737: image.png 

From this is seems it should ONLY be returning 0 (meaning don't join the line), 
if the "clause" in the (if,for,while) contains multiple conditions that have 
been broken over several lines.

I just don't think this is sufficiently constrained.

  // 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_MultiLine)
return 0;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[Kyrylo Bohdanenko via Phabricator via cfe-commits]

KyrBoh added a comment.

@MyDeveloperDay

> What it seems is "AllowShortIfStatementsOnASingleLine" ONLY seems to work 
> when there are no {}

AFAIR, there are 2 settings (in case of control statements):

- AllowShortBlocksOnASingleLine --> whether to treat (curly-)braced blocks as a 
single statement (i.e. allow them be one-liners)
- AllowShortIfStatementsOnASingleLine --> this actually tells whether control 
statements *can* be one-liners

So these two should be considered in pair. BTW, the documentation for 
AllowShortIfStatementsOnASingleLine seems to be a bit misleading

@curdeius 
Alright, I'll create a report


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

However if we add

  AllowShortBlocksOnASingleLine : Always

then it will be

  class Foo {
foo() {
  if (x) bar();
  if (x) { bar(); }
}
  };

but if we set

  BasedOnStyle: LLVM
  AllowShortIfStatementsOnASingleLine: Never
  ColumnLimit: 80
  AllowShortBlocksOnASingleLine : Always

we get

  class Foo {
foo() {
  if (x)
bar();
  if (x) {
bar();
  }
}
  };

which feels like `if(x){ bar(); }` is primarily controlled by  
`AllowShortBlocksOnASingleLine`  i.e. setting it to `Never` will override the 
`AllowShortIfStatementsOnASingleLine` setting for if statements with braces.

However if `AllowShortBlocksOnASingleLine` is `Always` then if 
`AllowShortIfStatementsOnASingleLine` is set to `Never` then it wins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM.
@KyrBoh, could you create a bug report for the problem you described, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D114521#3151431 , @curdeius wrote:

> LGTM, but I have one question. You mentioned in the bug ticket comment that 
> "this exposes a greater issue in AllowShortXXX". Have you found other cases 
> that misbehave? If so, then it would probably be good to add them.

So when I took a look I think I stumbled on what @KyrBoh said below, but I'm 
not convinced its related. (so really want to deal with this separately)

What it seems is "AllowShortIfStatementsOnASingleLine" ONLY seems to work when 
there are no `{}`  (this seems to be by design 
https://clang.llvm.org/docs/ClangFormatStyleOptions.html)

i.e. for

  BasedOnStyle: LLVM
  AllowShortIfStatementsOnASingleLine: AllIfsAndElse
  ColumnLimit: 80

I get

  class Foo {
foo() {
  if (x) bar();
  if (x) {
bar();
  }
}
  };

So I think that was just me not understanding that limitation, (although I 
agree it doesn't seem to follow other options of a similar name)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[Kyrylo Bohdanenko via Phabricator via cfe-commits]

KyrBoh added a comment.

In D114521#3151431 , @curdeius wrote:

> LGTM, but I have one question. You mentioned in the bug ticket comment that 
> "this exposes a greater issue in AllowShortXXX". Have you found other cases 
> that misbehave? If so, then it would probably be good to add them.

There also seems to be a behavior caused by AfterControlStatement = Multiline. 
This code:

if (condition) { contrinue; }

Is broken up into lines:

if (condition)
{

  continue;

}

I am not sure if this is intended behavior or not.

Because when having this configuration:

BreakBeforeBraces: Custom
BraceWrapping:

  AfterControlStatement: Never

AllowShortBlocksOnASingleLine: Always
AllowShortIfStatementsOnASingleLine: WithoutElse # any value except Never

clang-format would leave that code unchanged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

LGTM, but I have one question. You mentioned in the bug ticket comment that 
"this exposes a greater issue in AllowShortXXX". Have you found other cases 
that misbehave? If so, then it would probably be good to add them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114521

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

[PATCH] D114521: [clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: HazardyKnusperkeks, owenpan, curdeius, jaafar, 
KyrBoh.
MyDeveloperDay added projects: clang, clang-format.
MyDeveloperDay requested review of this revision.

https://bugs.llvm.org/show_bug.cgi?id=47936

Using the MultiLine setting for BraceWrapping.AfterControlStatement appears to 
disable AllowShortFunctionsOnASingleLine, even in cases without any control 
statements


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114521

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
@@ -2860,6 +2860,19 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(...){baz();}", Style));
+
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  Style.ColumnLimit = 80;
+  verifyFormat("void shortfunction() { bar(); }", Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  verifyFormat("void shortfunction()\n"
+   "{\n"
+   "  bar();\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, BeforeWhile) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -676,7 +676,7 @@
 // { <-- current Line
 //   baz();
 // }
-if (Line.First == Line.Last &&
+if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace) &&
 Style.BraceWrapping.AfterControlStatement ==
 FormatStyle::BWACS_MultiLine)
   return 0;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2860,6 +2860,19 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(...){baz();}", Style));
+
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  Style.ColumnLimit = 80;
+  verifyFormat("void shortfunction() { bar(); }", Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  verifyFormat("void shortfunction()\n"
+   "{\n"
+   "  bar();\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, BeforeWhile) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -676,7 +676,7 @@
 // { <-- current Line
 //   baz();
 // }
-if (Line.First == Line.Last &&
+if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace) &&
 Style.BraceWrapping.AfterControlStatement ==
 FormatStyle::BWACS_MultiLine)
   return 0;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits