[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe57f57841fbb: [clang-format] fix alignment w/o binpacked 
args (authored by cha5on, committed by curdeius).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17318,6 +17318,23 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://llvm.org/PR55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,14 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))
+  return true;
+
 // Continued braced list.
 if (ScopeStart > Start + 1 &&
 Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17318,6 +17318,23 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://llvm.org/PR55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,14 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))
+  return true;
+
 // Continued braced list.
  

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on added a comment.

In D125162#3510737 , @curdeius wrote:

> LGTM. Tell us if you need help landing this.

I don't have commit access, so some help would be great :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[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. Tell us if you need help landing this.
Thanks a lot for your contribution!




Comment at: clang/lib/Format/WhitespaceManager.cpp:374-375
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;

cha5on wrote:
> curdeius wrote:
> > It seems that we set `BK_BracedInit` only on `l_brace`, so no need for a 
> > redundant check.
> `BK_BracedInit` also gets set on `r_brace`, assuming I'm reading 
> `clang/lib/Format/UnwrappedLineParser.cpp` correctly.  I think we need both 
> to be true to be sure that this is the intended token.
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on added inline comments.



Comment at: clang/lib/Format/WhitespaceManager.cpp:374-375
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;

curdeius wrote:
> It seems that we set `BK_BracedInit` only on `l_brace`, so no need for a 
> redundant check.
`BK_BracedInit` also gets set on `r_brace`, assuming I'm reading 
`clang/lib/Format/UnwrappedLineParser.cpp` correctly.  I think we need both to 
be true to be sure that this is the intended token.



Comment at: clang/unittests/Format/FormatTest.cpp:17286
+  // BinPackArguments.
+  // See https://github.com/llvm/llvm-project/issues/55360
+  Alignment = getLLVMStyleWithColumns(50);

HazardyKnusperkeks wrote:
> I'd drop that, or at least replace it with https://llvm.org/PR55360.
Replaced.



Comment at: clang/unittests/Format/FormatTest.cpp:17290-17299
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name,\n"
+   " a_long_name},\n"

curdeius wrote:
> Why not just this?
I was reading a comment in this file that says that messUp manipulates leading 
whitespace and thought that meant it wouldn't be appropriate for this.  Upon a 
closer look, it seems to work fine here, so updating to just provide the 
expected case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on updated this revision to Diff 429138.
cha5on marked 4 inline comments as done.
cha5on added a comment.

Update for review.

- Further simplify testcases.
- Update issue URL in comment to use llvm.org alias.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,23 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://llvm.org/PR55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,14 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))
+  return true;
+
 // Continued braced list.
 if (ScopeStart > Start + 1 &&
 Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,23 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://llvm.org/PR55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,14 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))
+  return true;
+
 // Continued 

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:17286
+  // BinPackArguments.
+  // See https://github.com/llvm/llvm-project/issues/55360
+  Alignment = getLLVMStyleWithColumns(50);

I'd drop that, or at least replace it with https://llvm.org/PR55360.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Thanks for creating the bug report. A few more comments.




Comment at: clang/lib/Format/WhitespaceManager.cpp:374-375
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;

It seems that we set `BK_BracedInit` only on `l_brace`, so no need for a 
redundant check.



Comment at: clang/lib/Format/WhitespaceManager.cpp:394-398
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))

Ditto.



Comment at: clang/unittests/Format/FormatTest.cpp:17290-17299
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name,\n"
+   " a_long_name},\n"

Why not just this?



Comment at: clang/unittests/Format/FormatTest.cpp:17300-17309
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   "int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name,\n"
+   " a_long_name},\n"

And here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on marked 3 inline comments as done.
cha5on added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:17321
+
+verifyFormat(Expected, ToFormat, Alignment);
+  }

curdeius wrote:
> I don't think you need to create variables, just online the strings.
> 
> Also, could you minimize (and possibly split) as much as possible these test 
> cases?
Shrank this down and added a second testcase for direct-list-initialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on updated this revision to Diff 428262.
cha5on added a comment.

Update for review

- Add and link bug report.
- Remove braces per LLVM style.
- Shrink down test case and add second case for direct-list-initialization 
alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,33 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://github.com/llvm/llvm-project/issues/55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name,\n"
+   " a_long_name},\n"
+   " {a_longer_name_for_wrap,\n"
+   "a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   "int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name,\n"
+   " a_long_name},\n"
+   " {a_longer_name_for_wrap,\n"
+   "a_longer_name_for_wrap}};",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,14 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit))
+  return true;
+
 // Continued braced list.
 if (ScopeStart > Start + 1 &&
 Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,33 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  // See https://github.com/llvm/llvm-project/issues/55360
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});",
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name,\n"
+   " a_long_name},\n"
+   " {a_longer_name_for_wrap,\n"
+   "a_longer_name_for_wrap});",
+   Alignment);
+  verifyFormat("int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap}};",
+   "int a_long_name = 1;\n"
+   "auto b  = B{{a_long_name,\n"
+   " a_long_name},\n"
+   " {a_longer_name_for_wrap,\n"
+   "a_longer_name_for_wrap}};",
+   

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

FYI, I like to have a bug report because people that encounter the same problem 
(in an older release) can find it easily, and see if/when it was fixed. It also 
avoid working on the same thing by different people.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Marek Kurdej via Phabricator via cfe-commits]

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

Thanks for working on this!
Is there a bug report somewhere? If not, may you create one please?




Comment at: clang/lib/Format/WhitespaceManager.cpp:398
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit)) {
+  return true;

Elide braces please.



Comment at: clang/unittests/Format/FormatTest.cpp:17321
+
+verifyFormat(Expected, ToFormat, Alignment);
+  }

I don't think you need to create variables, just online the strings.

Also, could you minimize (and possibly split) as much as possible these test 
cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

For me there needs no bug report, but could you comment on the misformatting 
without the patch?




Comment at: clang/unittests/Format/FormatTest.cpp:17289
+  Alignment.BinPackArguments = false;
+  {
+const char *Expected = "struct A {\n"

Drop the scope, we don't have that in other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125162

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

[PATCH] D125162: [clang-format] fix alignment w/o binpacked args

[cha5on via Phabricator via cfe-commits]

cha5on created this revision.
Herald added a project: All.
cha5on requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The combination of

- AlignConsecutiveAssignments.Enabled = true
- BinPackArguments = false

would result in the first continuation line of a braced-init-list being
improperly indented (missing a shift) when in a continued function call.
Indentation was also wrong for braced-init-lists continuing a
direct-list-initialization.  Check for opening braced lists in
continuation and ensure that the correct shift occurs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125162

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,46 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  {
+const char *Expected = "struct A {\n"
+   "  int a;\n"
+   "  int b;\n"
+   "};\n"
+   "struct B {\n"
+   "  A a1;\n"
+   "  A a2;\n"
+   "};\n"
+   "int a_longer_name_for_wrap = 1;\n"
+   "// comment to prevent alignment\n"
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name, a_long_name},\n"
+   "{a_longer_name_for_wrap,\n"
+   " a_longer_name_for_wrap});";
+
+const char *ToFormat = "struct A {\n"
+   "int a;\n"
+   "int  b;\n"
+   "};\n"
+   "struct B{\n"
+   " A a1;\n"
+   "  A a2;\n"
+   "};\n"
+   "int a_longer_name_for_wrap = 1;\n"
+   "// comment to prevent alignment\n"
+   "int a_long_name = 1;\n"
+   "auto b  = B({a_long_name,\n"
+   " a_long_name},\n"
+   " {a_longer_name_for_wrap,\n"
+   "a_longer_name_for_wrap});";
+
+verifyFormat(Expected, ToFormat, Alignment);
+  }
 }
 
 TEST_F(FormatTest, AlignConsecutiveBitFields) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -371,6 +371,9 @@
 return false;
   if (Changes[ScopeStart].NewlinesBefore > 0)
 return false;
+  if (Changes[i].Tok->is(tok::l_brace) &&
+  Changes[i].Tok->is(BK_BracedInit))
+return true;
   return Style.BinPackArguments;
 }
 
@@ -387,6 +390,15 @@
 Changes[i].Tok->Previous->is(TT_ConditionalExpr))
   return true;
 
+// Continued direct-list-initialization using braced list.
+if (ScopeStart > Start + 1 &&
+Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(tok::l_brace) &&
+Changes[i].Tok->is(BK_BracedInit)) {
+  return true;
+}
+
 // Continued braced list.
 if (ScopeStart > Start + 1 &&
 Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17280,6 +17280,46 @@
   //  "ccc ? a : b,\n"
   //  "dd);",
   //  Alignment);
+
+  // Confirm proper handling of AlignConsecutiveAssignments with
+  // BinPackArguments.
+  Alignment = getLLVMStyleWithColumns(50);
+  Alignment.AlignConsecutiveAssignments.Enabled = true;
+  Alignment.BinPackArguments = false;
+  {
+const char *Expected = "struct A {\n"
+   "  int a;\n"
+   "  int b;\n"
+   "};\n"
+   "struct B {\n"
+   "  A a1;\n"
+   "  A a2;\n"
+   "};\n"
+