[clang-format] Treat ForEachMacros as loops

[curdeius]

Summary

TT_ForEachMacro should be considered in rules AllowShortBlocksOnASingleLine
and AllowShortLoopsOnASingleLine.
Fixes https://github.com/llvm/llvm-project/issues/45432.

[curdeius]

LGTM per se. But, as it's a somehow a breaking change, I'd rather wait for release 12 branch before landing to main, so that folks have time to veto before release 13.
Please update release notes.
Also given that @MyDeveloperDay was involved in the bug discussion, please wait for his review too (if he has anything to add).

[GoBigorGoHome]

Update Clang release notes

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

995–996

I'd like you to assert that short loops are off

[GoBigorGoHome]

@MyDeveloperDay

clang/unittests/Format/FormatTest.cpp

995–996

You mean make sure that AllowShortLoopsOnASingleLine is false?
The default style of verifyFormat is LLVM style where AllowShortLoopsOnASingleLine is already false.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

995–996

you should assert here to show that the previous tests were wrong.

[GoBigorGoHome]

Update tests

[curdeius]

LGTM. I let you coordinate with @MyDeveloperDay concerning the tests.

[MyDeveloperDay]

I'm never going to be the one to accept patches where people change tests without making it really clear why they are changing it. You have to prove you are not regressing behaviour, I work on the Beyonce rule, "if you liked it you should have put a test on it"

That means if you are changing that rule you are breaking what someone else did, so you have to work doubly hard to a) minimise the test changes and b) prove you haven't broken anything.

I'm always suspicious why people change a verifyFormat to an EXPECT_EQ to me it smells as if you tried it with verifyFormat and it didn't work so hacked a way around it.. ultimately this tends to leads to a bug further down the road.

clang/unittests/Format/FormatTest.cpp

996

verifyFormat

1008–1009

Please don't change existing tests without asserting why its ok to now do that.

I don't see a single assert that asserts that AllowShortLoopsOnASingleLine is true at this point

As you are now changing this behaviour you need to add a non short loop to prove that you haven't made a mistake for the non short loop case

[GoBigorGoHome]

@MyDeveloperDay
I changed the verifyFormat to EXPECT_NE because I don't know the proper way "to show that the previous tests were wrong", and I agree with you that it is a dirty hack. However, I think it is already clear why the tests were changed, that was because the short (empty) block followed an ForEachMacro was not wrapped previously and that is the very thing this patch wants to change.

Another thing that need to be clarified is that it seems that the rule AllowShortLoopsOnASingleLine only controls the case where the loop body is not enclosed in braces, e.g. for (int i = 0; i < 10; i++) std::cout << i; but not for (int i = 0; i < 10; i++) { std::cout << i;} so that AllowShortLoopsOnASingleLine is orthogonal to AllowShortBlocksOnAsingleLine. Can you confirm this?

In summary, (i) I am confused and need help in how to minimise the test changes for the purpose of this patch, and (ii) I agree that more tests are needed to prove that nothing has been broken.

In D94955#2536094, @MyDeveloperDay wrote:

I'm never going to be the one to accept patches where people change tests without making it really clear why they are changing it. You have to prove you are not regressing behaviour, I work on the Beyonce rule, "if you liked it you should have put a test on it"

That means if you are changing that rule you are breaking what someone else did, so you have to work doubly hard to a) minimise the test changes and b) prove you haven't broken anything.

I'm always suspicious why people change a verifyFormat to an EXPECT_EQ to me it smells as if you tried it with verifyFormat and it didn't work so hacked a way around it.. ultimately this tends to leads to a bug further down the road.

[MyDeveloperDay]

I guess I'm not making myself clear, I was just hoping you could be super explicit about what you are changing so anyone coming into this review would understand why the behaviour had changed (this is just pseudo code it may not be correct for all cases, but you should get the idea), We need to cover the SBS_Empty,SBS_Always,SBS_Never cases and the ShortLoops true/false

Add a normal for loop to the verifyFormat so we can see that FOREACH is being treated exactly the same as as a for(;;) loop

FormatStyle Style = getLLVMStyle();
// given that LLVM style is...
// AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
// AllowShortLoopsOnASingleLine = false;

// This test ensures that foreach blocks are treated exactly the same as their for(;;) counterparts.
EXPECT_EQ(Style.AllowShortBlocksOnASingleLine,FormatStyle::SBS_Never)
EXPECT_EQ(Style.AllowShortLoopsOnASingleLine ,true)

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}\n"
               "}",Style);

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

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


Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
Style.AllowShortLoopsOnASingleLine = false;
verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) {}\n"
               "  foreach (Item *item, itemlist) {}\n"
               "  Q_FOREACH (Item *item, itemlist) {}\n"
               "  BOOST_FOREACH (Item *item, itemlist) {}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) {}\n"
               "}",Style);

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

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

[curdeius]

@GoBigorGoHome, are you still interested in this review?

[curdeius]

Commandeering as it's stale..

[curdeius]

Add more tests. Fix missed cases.

[curdeius]

Assert + test for loop.

[curdeius]

clang/unittests/Format/FormatTest.cpp

996–1000

The existing tests have changed here. I added the tests case for a pure for loop to show the similarity.
I also check values of AllowShortBlocksOnASingleLine/AllowShortLoopsOnASingleLine.

1034

Euuh, I might need to so come clean up here.

[curdeius]

Clean up tests.

[curdeius]

clang/unittests/Format/FormatTest.cpp

1019

If wanted, I might commit the new tests without ForEachMacros before we follow on this review.

[MyDeveloperDay]

LGTM

clang/lib/Format/UnwrappedLineFormatter.cpp

tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
 
     // Try to merge a function block with left brace unwrapped
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
     // 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;
     }
     // Try to merge a control statement block with left brace wrapped
     if (I[1]->First->is(tok::l_brace) &&
         (TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
                                  tok::kw_switch, tok::kw_try, tok::kw_do,
if (I[1]->First->is(TT_FunctionLBrace) &&
       }
       return MergedLines;
     }
     if (TheLine->First->is(tok::kw_if)) {
       return Style.AllowShortIfStatementsOnASingleLine
                  ? 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;
     }
     if (TheLine->First->isOneOf(tok::kw_case, tok::kw_default)) {
       return Style.AllowShortCaseLabelsOnASingleLine
                  ? tryMergeShortCaseLabels(I, E, Limit)
                  : 0;
unsigned tryMergeSimpleControlStatement(
     if (!Line.First->is(tok::kw_do) && Line.Last->isNot(tok::r_paren))
       return 0;
     // Only merge do while if do is the only statement on the line.
     if (Line.First->is(tok::kw_do) && !Line.Last->is(tok::kw_do))
       return 0;
     if (1 + I[1]->Last->TotalLength > Limit)
       return 0;
     if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
-                             TT_LineComment))
+                             TT_ForEachMacro, TT_LineComment))
       return 0;
     // Only inline simple if's (no nested if or else), unless specified
     if (Style.AllowShortIfStatementsOnASingleLine != FormatStyle::SIS_Always) {
       if (I + 2 != E && Line.startsWith(tok::kw_if) &&
           I[2]->First->is(tok::kw_else))
         return 0;
     }
     return 1;
if (Line.First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_do, tok::kw_try,
         return 0;
       if (!Style.AllowShortIfStatementsOnASingleLine &&
           Line.startsWith(tok::kw_if) &&
           Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_Always &&
           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))
         return 0;
       // FIXME: Consider an option to allow short exception handling clauses on
       // a single line.
       // FIXME: This isn't covered by tests.
       // FIXME: For catch, __except, __finally the first token on the line

clang/unittests/Format/FormatTest.cpp

verifyFormat("for (auto aaaaaaaaaaaaaaaaaaaaa :\n"
                "     aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaa, aaaaaaaaaaaaa)) {\n}");
   verifyFormat("for (const aaaaaaaaaaaaaaaaaaaaa &aaaaaaaaa :\n"
                "     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");
   verifyFormat("for (aaaaaaaaa aaaaaaaaaaaaaaaaaaaaa :\n"
                "     aaaaaaaaaaaa.aaaaaaaaaaaa().aaaaaaaaa().a()) {\n}");
 }
 
 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"
+               "  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"
                "}");
 
   FormatStyle Style = getLLVMStyle();
   Style.SpaceBeforeParens =
       FormatStyle::SBPO_ControlStatementsExceptForEachMacros;
   verifyFormat("void f() {\n"
-               "  foreach(Item *item, itemlist) {}\n"
-               "  Q_FOREACH(Item *item, itemlist) {}\n"
-               "  BOOST_FOREACH(Item *item, itemlist) {}\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"
                "}",
                Style);
 
   // As function-like macros.
   verifyFormat("#define foreach(x, y)\n"
                "#define Q_FOREACH(x, y)\n"
                "#define BOOST_FOREACH(x, y)\n"
                "#define UNKNOWN_FOREACH(x, y)\n");
 
   // Not as function-like macros.
   verifyFormat("#define foreach (x, y)\n"
                "#define Q_FOREACH (x, y)\n"
                "#define BOOST_FOREACH (x, y)\n"
                "#define UNKNOWN_FOREACH (x, y)\n");
 
   // handle microsoft non standard extension
   verifyFormat("for each (char c in x->MyStringProperty)");
 }
 
 TEST_F(FormatTest, FormatsWhileLoop) {
   verifyFormat("while (true) {\n}");
   verifyFormat("while (true)\n"
                "  f();");
   verifyFormat("while () {\n}");
   verifyFormat("while () {\n"
                "  f();\n"
                "}");
TEST_F(FormatTest, StatementAttributeLikeMacros) {
   Style.StatementAttributeLikeMacros = {};
   EXPECT_EQ("void Foo::slot() {\n"
             "  unsigned char MyChar = 'x';\n"
             "  emit          signal(MyChar);\n"
             "  Q_EMIT        signal(MyChar);\n"
             "}",
             format(Source, Style));
 }
+
+TEST_F(FormatTest, TreatForEachMacrosAsLoops) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ForEachMacros.push_back("rng");
+  Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
+  verifyFormat("rng (i, 0, 10) { int j = 1; }", Style);
+  Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
+  verifyFormat("rng (i, 0, 10) {\n"
+               "  int j = 1;\n"
+               "}",
+               Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang