[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 4 inline comments as done.
galenelias added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:20888
+   "  {  7, 5,\"!!\" }\n"
+   "};\n",
+   Style);

owenpan wrote:
> galenelias wrote:
> > owenpan wrote:
> > > 
> > This is consistent with basically every single adjacent test in this 
> > function.  While I agree that this is unnecessary, in general I error on 
> > the side of consistency with the surrounding tests.  I'll defer to the 
> > maintainers, just wanted to make sure this is actually the preferred change 
> > given the numerous adjacent tests with this form.
> If you rebase your patch, you'll see that the trailing newlines in the 
> surrounding tests have been removed. (Even if they had not been removed, we 
> still wouldn't want new tests to have superfluous trailing newlines.)
Thanks for the information, looks like I missed your cleanup change by a day.  
In general some code-bases favor consistency over some extra gunk, so I tend to 
default to consistency, but good to know going forward.  I've rebased now.


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

https://reviews.llvm.org/D158795

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

[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 553809.

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

https://reviews.llvm.org/D158795

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
@@ -20879,6 +20879,16 @@
"});",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56,23, \"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  {  7, 5,\"!!\" }\n"
+   "};",
+   Style);
+
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
@@ -2,6 +21121,15 @@
"});",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56, 23,\"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  { 7,  5, \"!!\"}\n"
+   "};",
+   Style);
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -1226,6 +1226,7 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
@@ -1243,7 +1244,7 @@
   do {
 const FormatToken *Previous = Changes[Next->Index].Tok->Previous;
 if (Previous && Previous->isNot(TT_LineComment)) {
-  Changes[Next->Index].Spaces = 0;
+  Changes[Next->Index].Spaces = BracePadding;
   Changes[Next->Index].NewlinesBefore = 0;
 }
 Next = Next->NextColumnElement;
@@ -1276,7 +1277,7 @@
   NetWidth;
   if (Changes[CellIter->Index].NewlinesBefore == 0) {
 Changes[CellIter->Index].Spaces = (CellWidth - (ThisWidth + NetWidth));
-Changes[CellIter->Index].Spaces += (i > 0) ? 1 : 0;
+Changes[CellIter->Index].Spaces += (i > 0) ? 1 : BracePadding;
   }
   alignToStartOfCell(CellIter->Index, CellIter->EndIndex);
   for (const auto *Next = CellIter->NextColumnElement; Next;
@@ -1285,7 +1286,7 @@
 calculateCellWidth(Next->Index, Next->EndIndex, true) + NetWidth;
 if (Changes[Next->Index].NewlinesBefore == 0) {
   Changes[Next->Index].Spaces = (CellWidth - ThisWidth);
-  Changes[Next->Index].Spaces += (i > 0) ? 1 : 0;
+  Changes[Next->Index].Spaces += (i > 0) ? 1 : BracePadding;
 }
 alignToStartOfCell(Next->Index, Next->EndIndex);
   }
@@ -1299,12 +1300,13 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
   // The first cell needs to be against the left brace.
   if (Changes[CellIter->Index].NewlinesBefore == 0)
-Changes[CellIter->Index].Spaces = 0;
+Changes[CellIter->Index].Spaces = BracePadding;
   else
 Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
   ++CellIter;
@@ -1317,7 +1319,8 @@
 if (Changes[CellIter->Index].NewlinesBefore == 0) {
   Changes[CellIter->Index].Spaces =
   MaxNetWidth - ThisNetWidth +
-  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1
+ : BracePadding);
 }
 auto RowCount = 1U;
 auto Offset = std::distance(Cells.begin(), CellIter);
@@ -1331,7 +1334,7 @@
   if (Changes[Next->Index].NewlinesBefore == 0) {
 Changes[Next->Index].Spaces =
 MaxNetWidth - ThisNetWidth +
-(Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+(Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : BracePadding);
   }
   ++RowCount;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 553783.

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

https://reviews.llvm.org/D158795

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
@@ -20880,6 +20880,16 @@
"});\n",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56,23, \"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  {  7, 5,\"!!\" }\n"
+   "};\n",
+   Style);
+
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
@@ -21112,6 +21122,15 @@
"});\n",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56, 23,\"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  { 7,  5, \"!!\"}\n"
+   "};\n",
+   Style);
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -1226,6 +1226,7 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
@@ -1243,7 +1244,7 @@
   do {
 const FormatToken *Previous = Changes[Next->Index].Tok->Previous;
 if (Previous && Previous->isNot(TT_LineComment)) {
-  Changes[Next->Index].Spaces = 0;
+  Changes[Next->Index].Spaces = BracePadding;
   Changes[Next->Index].NewlinesBefore = 0;
 }
 Next = Next->NextColumnElement;
@@ -1276,7 +1277,7 @@
   NetWidth;
   if (Changes[CellIter->Index].NewlinesBefore == 0) {
 Changes[CellIter->Index].Spaces = (CellWidth - (ThisWidth + NetWidth));
-Changes[CellIter->Index].Spaces += (i > 0) ? 1 : 0;
+Changes[CellIter->Index].Spaces += (i > 0) ? 1 : BracePadding;
   }
   alignToStartOfCell(CellIter->Index, CellIter->EndIndex);
   for (const auto *Next = CellIter->NextColumnElement; Next;
@@ -1285,7 +1286,7 @@
 calculateCellWidth(Next->Index, Next->EndIndex, true) + NetWidth;
 if (Changes[Next->Index].NewlinesBefore == 0) {
   Changes[Next->Index].Spaces = (CellWidth - ThisWidth);
-  Changes[Next->Index].Spaces += (i > 0) ? 1 : 0;
+  Changes[Next->Index].Spaces += (i > 0) ? 1 : BracePadding;
 }
 alignToStartOfCell(Next->Index, Next->EndIndex);
   }
@@ -1299,12 +1300,13 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const int BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
   // The first cell needs to be against the left brace.
   if (Changes[CellIter->Index].NewlinesBefore == 0)
-Changes[CellIter->Index].Spaces = 0;
+Changes[CellIter->Index].Spaces = BracePadding;
   else
 Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
   ++CellIter;
@@ -1317,7 +1319,7 @@
 if (Changes[CellIter->Index].NewlinesBefore == 0) {
   Changes[CellIter->Index].Spaces =
   MaxNetWidth - ThisNetWidth +
-  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : BracePadding);
 }
 auto RowCount = 1U;
 auto Offset = std::distance(Cells.begin(), CellIter);
@@ -1331,7 +1333,7 @@
   if (Changes[Next->Index].NewlinesBefore == 0) {
 Changes[Next->Index].Spaces =
 MaxNetWidth - ThisNetWidth +
-(Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+(Changes[Next->Index].Tok->isNot(tok::r_brace) ? 1 : BracePadding);
   }
   ++RowCount;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

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



Comment at: clang/lib/Format/WhitespaceManager.cpp:1247
 if (Previous && Previous->isNot(TT_LineComment)) {
-  Changes[Next->Index].Spaces = 0;
+  Changes[Next->Index].Spaces = BracePadding;
   Changes[Next->Index].NewlinesBefore = 0;

owenpan wrote:
> owenpan wrote:
> > Can we assert that `Spaces == 0`? If not, we should add a test case.
> We can't assert that, but setting `Spaces` here seems superfluous as it's set 
> correctly below anyways?
I admit I'm not super confident on my understanding of this code, but this 
setting of Spaces is not redundant with any below settings.  If we set it to 
'3' for instance, that won't get overwritten later (because the other sets are 
all conditional, and don't hit for the `}` token).

So, I think this is still required (minus the issue of the existing 'Spaces' 
calculation from previous formatting pass seemingly already setting Spaces to 
the correct value).



Comment at: clang/unittests/Format/FormatTest.cpp:20888
+   "  {  7, 5,\"!!\" }\n"
+   "};\n",
+   Style);

owenpan wrote:
> 
This is consistent with basically every single adjacent test in this function.  
While I agree that this is unnecessary, in general I error on the side of 
consistency with the surrounding tests.  I'll defer to the maintainers, just 
wanted to make sure this is actually the preferred change given the numerous 
adjacent tests with this form.


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

https://reviews.llvm.org/D158795

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

[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

galenelias created this revision.
galenelias added a project: clang-format.
Herald added projects: All, clang.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
galenelias requested review of this revision.

Fixes: https://github.com/llvm/llvm-project/issues/57611

Currently AlignArrayOfStructures=Left is hard coding setting `Spaces` to
`0` for the token following the initial opening brace, but not touching
`Spaces` for the subsequent lines, which leads to the array being
misaligned.  Additionally, it's not adding a space before the trailing
`}` which is generally done when Cpp11BracedListStyle=false.

I'm not exactly sure why this function needs to override the `Spaces` as
it seems to generally already be set to either `0` or `1` according to
the other formatting settings, but I'm going with an explicit fix where
I just force the padding to `1` when Cpp11BracedListStyle=false.

AlignArrayOfStructures=Right doesn't have any alignment problems, but
isn't adding the expected padding around the braces either, so I'm
giving that the same treatment.


https://reviews.llvm.org/D158795

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
@@ -20880,6 +20880,16 @@
"});\n",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56,23, \"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  {  7, 5,\"!!\" }\n"
+   "};\n",
+   Style);
+
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
@@ -21112,6 +21122,15 @@
"});\n",
Style);
 
+  Style.Cpp11BracedListStyle = false;
+  verifyFormat("struct test demo[] = {\n"
+   "  { 56, 23,\"hello\" },\n"
+   "  { -1, 93463, \"world\" },\n"
+   "  { 7,  5, \"!!\"}\n"
+   "};\n",
+   Style);
+  Style.Cpp11BracedListStyle = true;
+
   Style.ColumnLimit = 0;
   EXPECT_EQ(
   "test demo[] = {\n"
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -1226,6 +1226,7 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const auto BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
@@ -1243,7 +1244,7 @@
   do {
 const FormatToken *Previous = Changes[Next->Index].Tok->Previous;
 if (Previous && Previous->isNot(TT_LineComment)) {
-  Changes[Next->Index].Spaces = 0;
+  Changes[Next->Index].Spaces = BracePadding;
   Changes[Next->Index].NewlinesBefore = 0;
 }
 Next = Next->NextColumnElement;
@@ -1276,7 +1277,7 @@
   NetWidth;
   if (Changes[CellIter->Index].NewlinesBefore == 0) {
 Changes[CellIter->Index].Spaces = (CellWidth - (ThisWidth + NetWidth));
-Changes[CellIter->Index].Spaces += (i > 0) ? 1 : 0;
+Changes[CellIter->Index].Spaces += (i > 0) ? 1 : BracePadding;
   }
   alignToStartOfCell(CellIter->Index, CellIter->EndIndex);
   for (const auto *Next = CellIter->NextColumnElement; Next;
@@ -1285,7 +1286,7 @@
 calculateCellWidth(Next->Index, Next->EndIndex, true) + NetWidth;
 if (Changes[Next->Index].NewlinesBefore == 0) {
   Changes[Next->Index].Spaces = (CellWidth - ThisWidth);
-  Changes[Next->Index].Spaces += (i > 0) ? 1 : 0;
+  Changes[Next->Index].Spaces += (i > 0) ? 1 : BracePadding;
 }
 alignToStartOfCell(Next->Index, Next->EndIndex);
   }
@@ -1299,12 +1300,13 @@
   if (!CellDescs.isRectangular())
 return;
 
+  const auto BracePadding = Style.Cpp11BracedListStyle ? 0 : 1;
   auto  = CellDescs.Cells;
   // Now go through and fixup the spaces.
   auto *CellIter = Cells.begin();
   // The first cell needs to be against the left brace.
   if (Changes[CellIter->Index].NewlinesBefore == 0)
-Changes[CellIter->Index].Spaces = 0;
+Changes[CellIter->Index].Spaces = BracePadding;
   else
 Changes[CellIter->Index].Spaces = CellDescs.InitialSpaces;
   ++CellIter;
@@ -1317,7 +1319,7 @@
 if (Changes[CellIter->Index].NewlinesBefore == 0) {
   Changes[CellIter->Index].Spaces =
   MaxNetWidth - ThisNetWidth +
-  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : 0);
+  (Changes[CellIter->Index].Tok->isNot(tok::r_brace) ? 1 : BracePadding);
 }
 auto RowCount = 1U;
 auto Offset = std::distance(Cells.begin(), 

[PATCH] D156705: [clang-format] Fix braced initializer formatting with templated base class initializer

[Galen Elias via Phabricator via cfe-commits]

galenelias added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:13461
   "  A() : a{} {}\n"
+  "  A() : Base{} {}\n"
   "  A(int b) : b(b) {}\n"

HazardyKnusperkeks wrote:
> Please also add nested templates.
Ok, added `A() : Base>{} {}` as a test case. I think that's what you 
mean by nested templates, but let me know if I misunderstood.


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

https://reviews.llvm.org/D156705

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

[PATCH] D156705: [clang-format] Fix braced initializer formatting with templated base class initializer

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 545798.
galenelias marked an inline comment as done.

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

https://reviews.llvm.org/D156705

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
@@ -13458,6 +13458,8 @@
   verifyFormat(
   "class A {\n"
   "  A() : a{} {}\n"
+  "  A() : Base{} {}\n"
+  "  A() : Base>{} {}\n"
   "  A(int b) : b(b) {}\n"
   "  A(int a, int b) : a(a), bs{{bs...}} { f(); }\n"
   "  int a, b;\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -581,7 +581,8 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   (NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
-   LBraceStack.back().PrevTok->is(tok::identifier));
+   LBraceStack.back().PrevTok->isOneOf(tok::identifier,
+   tok::greater));
 
   ProbablyBracedList =
   ProbablyBracedList ||


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13458,6 +13458,8 @@
   verifyFormat(
   "class A {\n"
   "  A() : a{} {}\n"
+  "  A() : Base{} {}\n"
+  "  A() : Base>{} {}\n"
   "  A(int b) : b(b) {}\n"
   "  A(int a, int b) : a(a), bs{{bs...}} { f(); }\n"
   "  int a, b;\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -581,7 +581,8 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   (NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
-   LBraceStack.back().PrevTok->is(tok::identifier));
+   LBraceStack.back().PrevTok->isOneOf(tok::identifier,
+   tok::greater));
 
   ProbablyBracedList =
   ProbablyBracedList ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156705: [clang-format] Fix braced initializer formatting with templated base class initializer

[Galen Elias via Phabricator via cfe-commits]

galenelias created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
galenelias requested review of this revision.

This fixes https://github.com/llvm/llvm-project/issues/64134 which
is a regression from https://reviews.llvm.org/D150403 where I
added logic to distinguish adjacent braces blocks between being braced
initializers or adjacent scopes by looking at the token before the first
opening brace to see if it is part of a class initializer list.

However, this logic fails to match initialization of a templated base
class initializer, because in that scenario the 'identifier' actually
ends in a `>`, so isn't detected.  I am extending the logic to just also
detect `>` explicitly to be recognized as the braced initializer syntax.
Hopefully there aren't many more holes in the detection logic here.


https://reviews.llvm.org/D156705

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
@@ -13458,6 +13458,7 @@
   verifyFormat(
   "class A {\n"
   "  A() : a{} {}\n"
+  "  A() : Base{} {}\n"
   "  A(int b) : b(b) {}\n"
   "  A(int a, int b) : a(a), bs{{bs...}} { f(); }\n"
   "  int a, b;\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -581,7 +581,8 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   (NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
-   LBraceStack.back().PrevTok->is(tok::identifier));
+   LBraceStack.back().PrevTok->isOneOf(tok::identifier,
+   tok::greater));
 
   ProbablyBracedList =
   ProbablyBracedList ||


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13458,6 +13458,7 @@
   verifyFormat(
   "class A {\n"
   "  A() : a{} {}\n"
+  "  A() : Base{} {}\n"
   "  A(int b) : b(b) {}\n"
   "  A(int a, int b) : a(a), bs{{bs...}} { f(); }\n"
   "  int a, b;\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -581,7 +581,8 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   (NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
-   LBraceStack.back().PrevTok->is(tok::identifier));
+   LBraceStack.back().PrevTok->isOneOf(tok::identifier,
+   tok::greater));
 
   ProbablyBracedList =
   ProbablyBracedList ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D150403#4539874 , @owenpan wrote:

> This seems to cause a regression. See 
> https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?

I will take a look.  The logic I added is trying to distinguish `{ } {` by 
looking at the token prior to the first l_brace, and checking if it's an 
identifier to quantify this as a braced list brace.  In this case, it's 
actually a `>`, so the code decides it's a scope brace instead.

I could easily patch it to also allow for a `>`, but don't want this to devolve 
into a long trail of hacks either.  Let me think about it a bit more, and I'll 
come up with a forward fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 543558.
galenelias added a comment.

Addresses latest review comments.


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19261,6 +19261,240 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyNoChange("switch (level) {\n"
+ "case log::info:return \"info\";\n"
+ "case log::warning: return \"warning\";\n"
+ "// comment\n"
+ "case log::critical: return \"critical\";\n"
+ "default:return \"default\";\n"
+ "\n"
+ "case log::severe: return \"severe\";\n"
+ "}",
+ Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 6 inline comments as done.
galenelias added a comment.

In D151761#4524653 , @owenpan wrote:

> FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` 
> instead of `AlignConsecutiveShortCaseStatements`.

My only hesitation with that name is that it might seem like something like 
there should be some alignment being applied to 'normal' consecutive case 
statements, which there isn't.  Maybe it's fine because the documentation makes 
it clear?  I'm definitely not picky about the name, whatever sounds idiomatic.  
@HazardyKnusperkeks, thoughts on just `AlignConsecutiveCaseStatements`?

  switch (level) {
  case 0:
  case 100:
 return "error";
  }


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4483050 , 
@HazardyKnusperkeks wrote:

> Thanks for the patience, I'm really looking forward to use this.
>
> But please wait for other opinions.

Thanks so much @HazardyKnusperkeks for all the feedback and help.  Out of 
curiosity, how long is typical to wait for additional opinions?  I just want to 
make sure this doesn't get lost.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 538288.
galenelias marked 2 inline comments as done.
galenelias added a comment.

Addressed outstanding review comments.


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,257 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 6 inline comments as done.
galenelias added inline comments.



Comment at: clang/include/clang/Format/Format.h:380
+}
+bool operator!=(const ShortCaseStatementsAlignmentStyle ) const {
+  return !(*this == R);

HazardyKnusperkeks wrote:
> I'd drop that. We don't have it for any other struct.
> 
> And with C++20 you don't need this anymore (although I don't know when llvm 
> will switch to c++20).
We do have it for `TrailingCommentsAlignmentStyle` and `AlignConsecutiveStyle`, 
but I will drop it.



Comment at: clang/unittests/Format/ConfigParseTest.cpp:321
   CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveDeclarations);
+  CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveShortCaseStatements);
 

HazardyKnusperkeks wrote:
> You now have to write your own checks for the parsing. (It's just copy & 
> paste.)
Aah, I had missed the `CHECK_PARSE_NESTED_BOOL`s for the existing 
alignConsecutive options within their macro.  Will fix. Thanks


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 538228.
galenelias marked an inline comment as done.
galenelias edited the summary of this revision.
galenelias added a comment.

This iteration switches away from using `AlignConsecutiveStyle` and instead 
uses a new `ShortCaseStatementsAlignmentStyle`.


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,257 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels
+  // don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n"
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it
+  // out.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Format/Format.h:327
+  /// \version 17
+  AlignConsecutiveStyle AlignConsecutiveShortCaseStatements;
 

HazardyKnusperkeks wrote:
> Since you are not using AlignTokens anymore, I'd say use your own struct. You 
> don't have the not used memberand you don't have to add a new member which is 
> not used by AlignTokens.
Sure. Will post an iteration with this - hopefully I'm getting all the plumbing 
correct.



Comment at: clang/lib/Format/WhitespaceManager.cpp:108
   alignConsecutiveAssignments();
+  alignConsecutiveShortCaseStatements();
   alignChainedConditionals();

HazardyKnusperkeks wrote:
> Is this the right position?
> Could you add a test with aligning assignments and case statements?
> 
> ```
> case XX: xx = 2; break;
> case X: x = 1; break;
> ```
> It should end up as
> ```
> case XX: xx = 2; break;
> case X:  x  = 1; break;
> ``` 
Great point, I hadn't actually really fully considered the ordering dependency 
here.  I agree that aligning short case statements before assignments (and 
probably declarations?) seems more correct.  However, these other 
alignConsecutive* methods don't actually align across scopes, so won't align 
declarations/assignments within the body of a case statement.  I verified that 
these don't align today using AlignConsecutiveAssignments.  I will however move 
this up to try to make it as logically correct as possible.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 537383.
galenelias edited the summary of this revision.
galenelias added a comment.

I re-wrote the alignment to stop using AlignTokens so that I can now handle all 
the edge cases that came up.  Specifically:

- Allowing empty case labels (implicit fall through) to not break the 
alignment, but only if they are sandwiched by short case statements.
- Don't align the colon of a non-short case label that follows short case 
labels when using 'AlignCaseColons=true'.
- Empty case labels will also now push out the alignment of the statements when 
using AlignCaseColons=false.

Also, this now avoids having to add the `IgnoreNestedScopes` parameter to 
`AlignTokens` which didn't feel great.

I refactored `AlignMacroSequence` so I could re-use the core aligning method, 
since it's the same logic I need, but I removed some of the dead code just to 
simplify things along the way.  But even with that, this version is quite a bit 
more code (~100 lines vs. ~30 lines).


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19244,6 +19244,255 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements push out the alignment, but non-short case labels don't.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::critical:\n"
+   "case log::warning:\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::extra_severe:\n" 
+   "  // comment\n"
+   "  return \"extra_severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements don't break the alignment, and potentially push it out
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "case log::critical:\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

Well, I guess I didn't put quite enough thought into the `AlignCaseColons` 
option.  While this solves the empty case label problem, it will also match in 
non-short case label scenarios such as the following, which doesn't seem 
desirable:

  switch (level) {
  case log::info  :
  case log::error :
return true;
  case log::none :
  case log::warning  :
  case log::critical :
return false;
  }

In order to solve this (and the other) issues, I think the only solution is to 
roll a custom alignment method instead of using `AlignTokens`, so that the 
alignment can be more correctly based on the detection of short case statements.

This is going to take considerably more time and code, so not sure when I'll be 
able to work on it.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 530707.
galenelias edited the summary of this revision.
galenelias added a comment.

Ok, I added the ability to align the case label colons.  In your original 
message you mentioned "I'd like to align the colon (and thus the statement 
behind that)" which implies actually adding the whitespace after the 'case' 
token itself.  Not sure if that would still be your preference in an ideal 
world, or if I just misinterpreted your request.  Aligning the colons 
themselves is very straightforward.

I opted to make this an option on `AlignConsecutiveStyle`, as that is 
consistent with how we customize some of the other AlignConsecutive* options, 
and it seemed awkward to add a floating top level boolean config option which 
applied to just this scenario - although it has the similar downside that it 
muddies the AlignConsecutiveStyle options for the other use cases.


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,228 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements (implicit fallthrough) currently break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+ 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4404179 , 
@HazardyKnusperkeks wrote:

> In D151761#4403828 , @galenelias 
> wrote:
>
>> In D151761#4400693 , 
>> @HazardyKnusperkeks wrote:
>>
>>> I'd say: align it.
>>
>> If it was straightforward, I would definitely agree to just align across 
>> empty case labels, but unfortunately there is no way to do that while using 
>> the generic AlignTokens (since the line with just a case has no token which 
>> matches the pattern, but needs to not break the alignment streak).  I guess 
>> the way to do it generically would be to add some sort of ExcludeLine lambda 
>> to allow filtering out lines.  Given that I don't think this is a common 
>> pattern with these 'Short Case Labels', and it's fairly easy to work around 
>> with `[[fallthrough]];` my plan is just to leave this as is (and add a test 
>> to make the behavior explicit).
>
> Leaving it open (and documenting that) I could get behind, but making it 
> explicit as desired behavior will not get my approval. On the other hand I 
> will not stand in the way, if the others approve.

Yah, I think leaving it open would be my preference at this point.  Not sure 
how to properly document that though?  Just be explicit in the tests?  Mention 
it in `alignConsecutiveShortCaseStatements`?  Should it be documented in the 
generated documentation (that feels a bit heavy)?


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 529365.

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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,181 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Empty case statements (implicit fallthrough) currently break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Implicit fallthrough cases can be aligned with either a comment or
+  // [[fallthrough]]
+  verifyFormat("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:  // fallthrough\n"
+   "case log::error:return \"error\";\n"
+   "case log::critical: /*fallthrough*/\n"
+   "case log::severe:   return \"severe\";\n"
+   "case log::diag: [[fallthrough]];\n"
+   "default:return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Verify trailing comment that needs a reflow also gets aligned properly.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: // fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: //fallthrough\n"
+   "case log::error:   return \"error\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+ 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4400693 , 
@HazardyKnusperkeks wrote:

> In D151761#4400056 , @galenelias 
> wrote:
>
>> In D151761#4394758 , 
>> @MyDeveloperDay wrote:
>>
>>> did you consider a case where the case falls through? (i.e. no return)
>>>
>>>   "case log::info :return \"info\";\n"
>>>   "case log::warning :\n"
>>>   "default :   return \"default\";\n"
>>
>> That's a great point.  I didn't really consider this, and currently this 
>> particular case won't align the case statements if they have an empty case 
>> block, however if you had some tokens (e.g. `// fallthrough`) it would.  
>> It's not immediately clear to me what the expectation would be.  I guess to 
>> align as if there was an invisible trailing token, but it's a bit awkward if 
>> the cases missing a body are the 'long' cases that push out the alignment.  
>> Also, I don't think it's possible to use `AlignTokens` and get this 
>> behavior, as there is no token on those lines to align, so it's not 
>> straightforward to handle.  I guess I'll be curious to see if there is 
>> feedback or cases where this behavior is desired, and if so, I can look into 
>> adding that functionality later.  Since right now it would involve a 
>> completely custom AlignTokens clone, my preference would be to just leave 
>> this as not supported.
>
> I'd say: align it.

If it was straightforward, I would definitely agree to just align across empty 
case labels, but unfortunately there is no way to do that while using the 
generic AlignTokens (since the line with just a case has no token which matches 
the pattern, but needs to not break the alignment streak).  I guess the way to 
do it generically would be to add some sort of ExcludeLine lambda to allow 
filtering out lines.  Given that I don't think this is a common pattern with 
these 'Short Case Labels', and it's fairly easy to work around with 
`[[fallthrough]];` my plan is just to leave this as is (and add a test to make 
the behavior explicit).

In D151761#4402568 , @MyDeveloperDay 
wrote:

> In D151761#4400056 , @galenelias 
> wrote:
>
>> In D151761#4394758 , 
>> @MyDeveloperDay wrote:
>>
>>> did you consider a case where the case falls through? (i.e. no return)
>>>
>>>   "case log::info :return \"info\";\n"
>>>   "case log::warning :\n"
>>>   "default :   return \"default\";\n"
>>
>> That's a great point.  I didn't really consider this, and currently this 
>> particular case won't align the case statements if they have an empty case 
>> block, however if you had some tokens (e.g. `// fallthrough`) it would.  
>> It's not immediately clear to me what the expectation would be.  I guess to 
>> align as if there was an invisible trailing token, but it's a bit awkward if 
>> the cases missing a body are the 'long' cases that push out the alignment.  
>> Also, I don't think it's possible to use `AlignTokens` and get this 
>> behavior, as there is no token on those lines to align, so it's not 
>> straightforward to handle.  I guess I'll be curious to see if there is 
>> feedback or cases where this behavior is desired, and if so, I can look into 
>> adding that functionality later.  Since right now it would involve a 
>> completely custom AlignTokens clone, my preference would be to just leave 
>> this as not supported.
>
> Can you add a unit test with a // fallthough comment, and a /* FALLTHROUGH */ 
> and [[fallthrough]] so we know whats going to happen in those cases at a 
> minimum.

Done.  This actually caught an issue I wasn't aware of where if you use 
`//fallthrough` and the comment needs to be reflowed, you get two `Change` 
entries pointing to the same comment token, which throws off the algorithm, and 
results in broken alignment.  Attempted to fix it up to ignore the inner reflow 
change and it seems to work in my basic validation.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias marked an inline comment as done.
galenelias added a comment.

In D151761#4394758 , @MyDeveloperDay 
wrote:

> did you consider a case where the case falls through? (i.e. no return)
>
>   "case log::info :return \"info\";\n"
>   "case log::warning :\n"
>   "default :   return \"default\";\n"

That's a great point.  I didn't really consider this, and currently this 
particular case won't align the case statements if they have an empty case 
block, however if you had some tokens (e.g. `// fallthrough`) it would.  It's 
not immediately clear to me what the expectation would be.  I guess to align as 
if there was an invisible trailing token, but it's a bit awkward if the cases 
missing a body are the 'long' cases that push out the alignment.  Also, I don't 
think it's possible to use `AlignTokens` and get this behavior, as there is no 
token on those lines to align, so it's not straightforward to handle.  I guess 
I'll be curious to see if there is feedback or cases where this behavior is 
desired, and if so, I can look into adding that functionality later.  Since 
right now it would involve a completely custom AlignTokens clone, my preference 
would be to just leave this as not supported.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

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



Comment at: clang/docs/ClangFormatStyleOptions.rst:798
+
+case log::info: return "info:";
+case log::warning:  return "warning:";

MyDeveloperDay wrote:
> Do you think the documentation should give examples of the the other cases?
Hmm, I'm not sure what other cases?  Cases where it doesn't align?  In general, 
for the 'Align*' options we only show examples of the resulting formatting.



Comment at: clang/docs/ClangFormatStyleOptions.rst:790
+
+**AlignConsecutiveShortCaseLabels** (``AlignConsecutiveStyle``) 
:versionbadge:`clang-format 17` :ref:`¶ `
+  Style of aligning consecutive short case labels.

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > galenelias wrote:
> > > > MyDeveloperDay wrote:
> > > > > Did you generate this by hand or run the dump_format_style.py 
> > > > > function? Format.h and the rst look out of sync
> > > > This was generated from dump_format_style.py.  What looks out of sync?  
> > > > My guess is the confusing thing is all the styles which use 
> > > > `AlignConsecutiveStyle` get the same documentation pasted for them 
> > > > which specifically references `AlignConsecutiveMacros` and makes it 
> > > > look like it's for the wrong option.
> > > that doesn't feel right to me.. not saying its not dump_format_style.py 
> > > but it shouldn't do that in my view
> > What shouldn't it be doing?
> > The struct is used for multiple options and the documentation is that ways 
> > since we have the struct.
> I understand why its there, I just don't like that the text is repeated and 
> doesn't really reference the case statements, (it could be confusing).
> 
> for example what would `PadOperators` mean in this case
> 
> 
I agree that it's confusing, I've actually gotten confused a few times while 
reading the documentation for various 'AlignConsecutive*' options.  
PadOperators and AlignCompound both explicitly state they only apply to 
`AlignConsecutiveAssignments`.  Not sure what the fix would be, as this seems 
to be a fairly by design aspect of the struct sharing with regards to the 
documentation generation.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"

HazardyKnusperkeks wrote:
> If both strings are the same, you only need to supply it once.
> 
> Or are they different and I can't see it?
They are the same, but I can't use the single string version, as that one calls 
test::messUp on the string which will strip blank lines, which then invalidates 
the test case.  It seems pretty much all tests which are trying to validate 
behavior around empty spaces has to use the two string version.



Comment at: clang/unittests/Format/FormatTest.cpp:19257
+
+  Alignment.ColumnLimit = 80;
+  Alignment.SpaceBeforeCaseColon = true;

HazardyKnusperkeks wrote:
> For what is this?
Oops, I was trying to set ColumnLimit = 0 before to stop it from merging 
multi-line case statements, but it wasn't working as expected, so I ended up 
inserting comments instead, and forgot to remove resetting this.  Good catch!



Comment at: clang/unittests/Format/FormatTest.cpp:19206
+
+  // Make sure we don't incorrectly align correctly across nested switch cases
+  EXPECT_EQ("switch (level) {\n"

HazardyKnusperkeks wrote:
> 
I'll get used to adding full stops eventually... sorry for not catching this.


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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 528118.
galenelias added a comment.

Fixup up review comments.


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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,147 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+   "}",
+   Alignment);
+  Alignment.SpaceBeforeCaseColon = false;
+
+  // Make sure we don't incorrectly align correctly across nested switch cases.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other:\n"
+   "  switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "  }\n"
+   "  break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other: switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "}\n"
+   "break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment);
+
+  Alignment.AlignConsecutiveShortCaseStatements.AcrossEmptyLines = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:   

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 528072.
galenelias marked 3 inline comments as done.
galenelias retitled this revision from "clang-format: Add 
AlignConsecutiveShortCaseLabels" to "clang-format: Add 
AlignConsecutiveShortCaseStatements".
galenelias edited the summary of this revision.

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

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,148 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseStatements) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseStatements.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "case log::warning:\n"
+   "  return \"warning\";\n"
+   "}",
+   Alignment);
+
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "// comment\n"
+   "case log::critical: return \"critical\";\n"
+   "default:return \"default\";\n"
+   "\n"
+   "case log::severe: return \"severe\";\n"
+   "}",
+   Alignment);
+
+  // Verify adjacent non-short case statements don't change the alignment, and
+  // properly break the set of consecutive statements.
+  verifyFormat("switch (level) {\n"
+   "case log::critical:\n"
+   "  // comment\n"
+   "  return \"critical\";\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:\n"
+   "  // comment\n"
+   "  return \"\";\n"
+   "case log::error:  return \"error\";\n"
+   "case log::severe: return \"severe\";\n"
+   "case log::extra_critical:\n"
+   "  // comment\n"
+   "  return \"extra critical\";\n"
+   "}",
+   Alignment);
+
+  Alignment.ColumnLimit = 80;
+  Alignment.SpaceBeforeCaseColon = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info :return \"info\";\n"
+   "case log::warning : return \"warning\";\n"
+   "default :   return \"default\";\n"
+   "}",
+   Alignment);
+  Alignment.SpaceBeforeCaseColon = false;
+
+  // Make sure we don't incorrectly align correctly across nested switch cases.
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other:\n"
+   "  switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "  }\n"
+   "  break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   "switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other: switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "}\n"
+   "break;\n"
+   "case log::error: return \"error\";\n"
+   "default: 

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseLabels

[Galen Elias via Phabricator via cfe-commits]

galenelias added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:790
+
+**AlignConsecutiveShortCaseLabels** (``AlignConsecutiveStyle``) 
:versionbadge:`clang-format 17` :ref:`¶ `
+  Style of aligning consecutive short case labels.

MyDeveloperDay wrote:
> Did you generate this by hand or run the dump_format_style.py function? 
> Format.h and the rst look out of sync
This was generated from dump_format_style.py.  What looks out of sync?  My 
guess is the confusing thing is all the styles which use 
`AlignConsecutiveStyle` get the same documentation pasted for them which 
specifically references `AlignConsecutiveMacros` and makes it look like it's 
for the wrong option.



Comment at: clang/unittests/Format/FormatTest.cpp:19255
+   Alignment);
+}
+

HazardyKnusperkeks wrote:
> Add a test with `AcrossEmptyLines` and `AcrossComments`.
There is a test for both `AcrossEmptyLines` and `AcrossComments`.  Maybe I'm 
not understanding your comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseLabels

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4385163 , 
@HazardyKnusperkeks wrote:

> When I read the title I thought "Yay, some one is doing what I want and don't 
> find the time.", but (for me) sadly you're not.
> I'd like to align the colon (and thus the statement behind that). Do you 
> think you can add that option too?

Hmm, it had not occurred to me that there are folks who would want to insert 
the spaces after the 'case'.  I can think about how that might be accomplished, 
but it doesn't seem straightforward given the current alignment infrastructure 
since the token you want to align (the colon) isn't the token which should be 
padded (the token after the case keyword).  I think I'd have to write custom 
alignment code to allow for the separation of these two.




Comment at: clang/include/clang/Format/Format.h:308
+  /// \version 17
+  AlignConsecutiveStyle AlignConsecutiveShortCaseLabels;
 

HazardyKnusperkeks wrote:
> We need another name, you're not aligning the label, but the statement.
I was taking the name from the GitHub issue, which while I agree is a slight 
misnomer seemed to yield what I assumed most people would expect.  I guess 
`AlignConsecutiveStatementsAfterShortCaseLabels` is more correct, but is also 
getting pretty verbose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151761

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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseLabels

[Galen Elias via Phabricator via cfe-commits]

galenelias created this revision.
galenelias added a project: clang-format.
Herald added projects: All, clang.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
galenelias requested review of this revision.

This resolves https://github.com/llvm/llvm-project/issues/55475.

This adds a new `AlignConsecutiveShortCaseLabels` option in line with the 
existing AlignConsecutiveStyle options, which when 
`AllowShortCaseLabelsOnASingleLine` is enabled, will align the tokens after the 
case statement's colon.

For the implementation, I went with re-using the `AlignTokens` method, however 
since consecutive case labels require traversing scopes, the default behavior 
to recurse upon entering a new scope (and breaking formatting across scopes) 
doesn't work.  I opted to add a new `IgnoreNestedScopes` parameter to prevent 
this scope traversal, but it feels a touch hacky.

The other options would be to either write a new version of the alignment code, 
or to try to leverage one of the other existing aligning functions (seems the 
other one-off ones are `alignTrailingComments` and `alignConsecutiveMacros`).  
Using the logic from `WhitespaceManager::alignConsecutiveMacros` technically 
seemed to work here, so I could factor the token matching function out of that 
and re-use it without any other edits, but other than not traversing scopes, I 
wasn't quite sure what special logic it was doing that might not make sense to 
re-use generically, so opted for trying to leverage the more general purpose 
AlignTokens instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151761

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19192,6 +19192,68 @@
BracedAlign);
 }
 
+TEST_F(FormatTest, AlignConsecutiveShortCaseLabels) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AllowShortCaseLabelsOnASingleLine = true;
+  Alignment.AlignConsecutiveShortCaseLabels.Enabled = true;
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "default:   return \"default\";\n"
+   "}",
+   Alignment);
+
+  // Make sure we don't incorrectly align correctly across nested switch cases
+  EXPECT_EQ("switch (level) {\n"
+"case log::info:return \"info\";\n"
+"case log::warning: return \"warning\";\n"
+"case log::other:\n"
+"  switch (sublevel) {\n"
+"  case log::info:return \"info\";\n"
+"  case log::warning: return \"warning\";\n"
+"  }\n"
+"  break;\n"
+"case log::error: return \"error\";\n"
+"default: return \"default\";\n"
+"}",
+format("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "case log::warning: return \"warning\";\n"
+   "case log::other: switch (sublevel) {\n"
+   "  case log::info:return \"info\";\n"
+   "  case log::warning: return \"warning\";\n"
+   "}\n"
+   "break;\n"
+   "case log::error: return \"error\";\n"
+   "default: return \"default\";\n"
+   "}",
+   Alignment));
+
+  Alignment.AlignConsecutiveShortCaseLabels.AcrossEmptyLines = true;
+
+  EXPECT_EQ("switch (level) {\n"
+"case log::info:return \"info\";\n"
+"\n"
+"case log::warning: return \"warning\";\n"
+"}",
+format("switch (level) {\n"
+   "case log::info: return \"info\";\n"
+   "\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   Alignment));
+
+  Alignment.AlignConsecutiveShortCaseLabels.AcrossEmptyLines = false;
+  Alignment.AlignConsecutiveShortCaseLabels.AcrossComments = true;
+
+  verifyFormat("switch (level) {\n"
+   "case log::info:return \"info\";\n"
+   "//\n"
+   "case log::warning: return \"warning\";\n"
+   "}",
+   Alignment);
+}
+
 TEST_F(FormatTest, AlignWithLineBreaks) {
   auto Style = getLLVMStyleWithColumns(120);
 
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- 

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 524484.
galenelias edited the summary of this revision.
galenelias added a comment.

Thanks for the additional review comments.  Hopefully everything if fixed.

In D150403#4358845 , @owenpan wrote:

> Can you put your authorship in the 'John Doe ' 
> 
>  format?

Galen Elias 

I'm not sure if there is somewhere I should be putting that in the summary or 
diff itself, or just here in the comments?


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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated. It doesn't seem to cause a problem.
+  // So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
  

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D150403#4347323 , 
@HazardyKnusperkeks wrote:

> We'll wait a bit, if someone might have a comment. And (at least I) need name 
> and email for the commit.

Name: Galen Elias
Email: galenelias at gmail dot com


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

Thanks @HazardyKnusperkeks!  I don't have commit access, so will need someone 
to land this for me.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias marked 3 inline comments as done.
galenelias added a comment.

Thanks for the review and feedback.  Sorry about the unfortunate timing between 
@sstwcw and my fix - we submitted our fixes less than 24 hours apart.  I didn't 
think there would be another simultaneous fix for this 5 year old bug, so 
didn't even think to link to the review in the GitHub issue.  I will definitely 
do this for any future contributions.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 522245.
galenelias edited the summary of this revision.
galenelias added a comment.

Addressed remaining review feedback.


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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated.  It doesn't seem to cause a
+  // problem.  So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor 

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

@HazardyKnusperkeks, thanks for the feedback.  I added the TokenAnnotatorTests 
from @sstwcw's review.  I also updated the design to use a single stack instead 
of the two parallel stacks.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias updated this revision to Diff 521828.

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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated.  It doesn't seem to cause a
+  // problem.  So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor initializer list
+  // followed by constructor body, or just adjacent blocks
+  ProbablyBracedList =
+  

[PATCH] D150452: [clang-format] Recognize nested blocks

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

Coincidentally I also sent out a review to fix this issue yesterday, but went 
with a different approach of trying to scope the ProbablyBracedList logic by 
just looking at the lbrace previous token.

https://reviews.llvm.org/D150403


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150452

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias added a subscriber: sstwcw.
galenelias added a comment.

Looks like @sstwcw also submitted a fix for the same issue, with a bit of a 
different approach: https://reviews.llvm.org/D150452


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[Galen Elias via Phabricator via cfe-commits]

galenelias created this revision.
galenelias added reviewers: cpplearner, HazardyKnusperkeks, MyDeveloperDay.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, owenpan.
galenelias requested review of this revision.

This is a retry of https://reviews.llvm.org/D114583, which was backed
out for regressions. This fixes
https://github.com/llvm/llvm-project/issues/33891.

Clang Format is detecting a nested scope followed by another open brace
as a braced initializer list due to incorrectly thinking it's matching a
braced initializer at the end of a constructor initializer list which is
followed by the body open brace.

Unfortunately, UnwrappedLineParser isn't doing a very detailed parse, so
it's not super straightforward to distinguish these cases given the
current structure of calculateBraceTypes. My current hypothesis is that
these can be disambiguated by looking at the token preceding the
l_brace, as initializer list parameters will be preceded by an
identifier, but a scope block generally will not (barring the MACRO
wildcard).

To this end, I am tracking the previous token type in a stack parallel
to LBraceStack to help scope this particular case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150403

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
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -492,6 +492,9 @@
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
   SmallVector LBraceStack;
+  // Track the previous token type corresponding to our lbraces // to help
+  // detect brace types
+  SmallVector PrevTokenKindStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -522,6 +525,8 @@
 Tok->setBlockKind(BK_Unknown);
   }
   LBraceStack.push_back(Tok);
+  PrevTokenKindStack.push_back(PrevTok ? PrevTok->Tok.getKind()
+   : tok::unknown);
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
@@ -570,8 +575,13 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor initializer list
+  // followed by constructor body, or just adjacent blocks
+  ProbablyBracedList = ProbablyBracedList ||
+   NextTok->is(tok::l_brace) &&
+   PrevTokenKindStack.back() == 
tok::identifier;
 
   ProbablyBracedList =
   ProbablyBracedList ||
@@ -602,6 +612,7 @@
 }
   }
   LBraceStack.pop_back();
+  PrevTokenKindStack.pop_back();
   break;
 case tok::identifier:
   if (!Tok->is(TT_StatementMacro))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+