LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 46087 - AllowShortBlocksOnASingleLine fails on ForEachMacros
Summary: AllowShortBlocksOnASingleLine fails on ForEachMacros
Status: CONFIRMED
Alias: None
Product: clang
Classification: Unclassified
Component: Formatter (show other bugs)
Version: 10.0
Hardware: PC Windows NT
: P enhancement
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-26 19:01 PDT by Jiashu Zou
Modified: 2021-01-19 22:51 PST (History)
6 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jiashu Zou 2020-05-26 19:01:58 PDT
.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; 
}
Comment 1 MyDeveloperDay 2020-05-27 09:45:24 PDT
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))
Comment 2 Jiashu Zou 2020-05-27 10:06:49 PDT
(In reply to MyDeveloperDay from comment #1)
> You are correct the TT_ForEachMacro is not being considered in the rules for 
> 
> AllowShortBlocksOnASingleLine: Always
> AllowShortLoopsOnASingleLine: true


According to the clang-format [doc](http://clang.llvm.org/docs/ClangFormatStyleOptions.html), 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.
Comment 3 MyDeveloperDay 2020-05-29 09:11:56 PDT
You are correct, I worked my way around that too..but thats 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.
Comment 4 Marek Kurdej 2021-01-19 22:51:24 PST
For the record, review in progress: https://reviews.llvm.org/D94955.