AllowShortBlocksOnASingleLine fails on ForEachMacros

[GoBigorGoHome]

Bugzilla Link	46087
Version	10.0
OS	Windows NT
CC	@mkurdej,@mydeveloperday,@GoBigorGoHome

Extended Description

.clang-format:

ForEachMacros: ['rng']
AllowShortBlocksOnASingleLine: Never

Before:

rng (i, 0, 10) { 
  std::cout << i; 
}

After:

rng (i, 0, 10) { std::cout << i; }

Expect:

rng (i, 0, 10) { 
  std::cout << i; 
}

[mydeveloperday]

You are correct the TT_ForEachMacro is not being considered in the rules for

AllowShortBlocksOnASingleLine: Always
AllowShortLoopsOnASingleLine: true

--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -301,7 +301,7 @@ private:
     }
     // Try to merge a control statement block with left brace unwrapped
     if (TheLine->Last->is(tok::l_brace) && TheLine->First != TheLine->Last &&
-        TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
+        TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,TT_ForEachMacro)) {
       return Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
@@ -346,7 +346,7 @@ private:
     // statement token
     if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
         I != AnnotatedLines.begin() &&
-        I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
+        I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, TT_ForEachMacro)) {
       unsigned MergedLines = 0;
       if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
         MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
@@ -406,7 +406,7 @@ private:
                  ? tryMergeSimpleControlStatement(I, E, Limit)
                  : 0;
     }
-    if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while, tok::kw_do)) {
+    if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while, tok::kw_do,TT_ForEachMacro)) {
       return Style.AllowShortLoopsOnASingleLine
                  ? tryMergeSimpleControlStatement(I, E, Limit)
                  : 0;
@@ -563,12 +563,12 @@ private:
           I + 2 != E && !I[2]->First->is(tok::r_brace))
         return 0;
       if (!Style.AllowShortLoopsOnASingleLine &&
-          Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
+          Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for,TT_ForEachMacro) &&
           !Style.BraceWrapping.AfterControlStatement &&
           !I[1]->First->is(tok::r_brace))
         return 0;
       if (!Style.AllowShortLoopsOnASingleLine &&
-          Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
+          Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for,TT_ForEachMacro) &&
           Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_Always &&
           I + 2 != E && !I[2]->First->is(tok::r_brace))

[GoBigorGoHome]

You are correct the TT_ForEachMacro is not being considered in the rules for

AllowShortBlocksOnASingleLine: Always
AllowShortLoopsOnASingleLine: true

According to the clang-format doc, it seems that AllowShortLoopsOnASingleLine: true only applies to

while (true)
  continue;

but not to

while (true) {
  continue;
}

If this is the intended behavior, then only the AllowShortBlocksOnASingleLine option is relevant.

[mydeveloperday]

You are correct, I worked my way around that too..but thanks for stating that here.

I looked further into this, we can add this I feel. but, the existing tests are wrong.

The following is present for a situation where we have LLVM style

TEST_F(FormatTest, ForEachLoops) {
  verifyFormat("void f() {\n"
               "  foreach (Item *item, itemlist) {}\n"
               "  Q_FOREACH (Item *item, itemlist) {}\n"
               "  BOOST_FOREACH (Item *item, itemlist) {}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) {}\n"
               "}");

But actually if they are going to abide by the rules of for loops then all of these the tests need to be change to match the basic for loop, but not for UNKNOWN_FORACH which is not a know ForEachMacro

  verifyFormat("void f() {\n"
               "  for (int i=0;i<10;i++) {\n  }\n"
               "  foreach (Item *item, itemlist) {\n  }\n"
               "  Q_FOREACH (Item *item, itemlist) {\n  }\n"
               "  BOOST_FOREACH (Item *item, itemlist) {\n  }\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) { }\n"
               "}");

This isn't a big change but its a breaking change, I think we can propose it but we might get pushback.

[mkurdej]

For the record, review in progress: https://reviews.llvm.org/D94955.

[mydeveloperday]

as there is no further action on this fix since Jan 2021, I'll take over this review.