Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AllowShortBlocksOnASingleLine fails on ForEachMacros #45432

Closed
GoBigorGoHome mannequin opened this issue May 27, 2020 · 5 comments
Closed

AllowShortBlocksOnASingleLine fails on ForEachMacros #45432

GoBigorGoHome mannequin opened this issue May 27, 2020 · 5 comments
Assignees
@mydeveloperday
Labels
bug Indicates an unexpected problem or unintended behavior bugzilla Issues migrated from bugzilla clang-format

Comments

@GoBigorGoHome
Copy link
Mannequin

GoBigorGoHome mannequin commented May 27, 2020 •
edited by mydeveloperday

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
Copy link
Contributor

mydeveloperday commented May 27, 2020 •
edited

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
Copy link
Mannequin Author

GoBigorGoHome mannequin commented May 27, 2020 •
edited by mydeveloperday

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
Copy link
Contributor

mydeveloperday commented May 29, 2020 •
edited

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
Copy link
Member

mkurdej commented Jan 20, 2021

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

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@mydeveloperday mydeveloperday added beginner bug Indicates an unexpected problem or unintended behavior labels Dec 15, 2021
@mydeveloperday
Copy link
Contributor

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

@mydeveloperday mydeveloperday self-assigned this Dec 15, 2021
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@mkurdej
[clang-format] Treat ForEachMacros as loops
8a7eb01 
TT_ForEachMacro should be considered in rules AllowShortBlocksOnASingleLine
and AllowShortLoopsOnASingleLine.
Fixes llvm/llvm-project#45432.

Reviewed By: MyDeveloperDay

Differential Revision: https://reviews.llvm.org/D94955
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mydeveloperday mydeveloperday

Labels
bug Indicates an unexpected problem or unintended behavior bugzilla Issues migrated from bugzilla clang-format
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mkurdej @mydeveloperday