[clang] 9cb9445 - [clang-format] Correctly format loops and `if` statements even if preceded with comments.

[Marek Kurdej via cfe-commits]

Author: Marek Kurdej
Date: 2022-02-13T21:22:17+01:00
New Revision: 9cb944597907458ce9c2f6bd0ecc9723b674b77f

URL: https://github.com/llvm/llvm-project/commit/9cb944597907458ce9c2f6bd0ecc9723b674b77f
DIFF: https://github.com/llvm/llvm-project/commit/9cb944597907458ce9c2f6bd0ecc9723b674b77f.diff

LOG: [clang-format] Correctly format loops and `if` statements even if preceded with comments.

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

Braces in loops and in `if` statements with leading (block) comments were formatted according to `BraceWrapping.AfterFunction` and not `AllowShortBlocksOnASingleLine`/`AllowShortLoopsOnASingleLine`/`AllowShortIfStatementsOnASingleLine`.

Previously, the code:
```
while (true) {
  f();
}
/*comment*/ while (true) {
  f();
}
```

was incorrectly formatted to:
```
while (true) {
  f();
}
/*comment*/ while (true) { f(); }
```

when using config:
```
BasedOnStyle: LLVM
BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: false
AllowShortBlocksOnASingleLine: false
AllowShortLoopsOnASingleLine: false
```

and it was (correctly but by chance) formatted to:
```
while (true) {
  f();
}
/*comment*/ while (true) {
  f();
}
```

when using enabling brace wrapping after functions:
```
BasedOnStyle: LLVM
BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true
AllowShortBlocksOnASingleLine: false
AllowShortLoopsOnASingleLine: false
```

Reviewed By: MyDeveloperDay, HazardyKnusperkeks, owenpan

Differential Revision: https://reviews.llvm.org/D119649

Added: 
    

Modified: 
    clang/lib/Format/UnwrappedLineFormatter.cpp
    clang/unittests/Format/FormatTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 88efda487eeb6..883030cb3dc16 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -319,6 +319,15 @@ class LineJoiner {
 
     bool MergeShortFunctions = ShouldMergeShortFunctions();
 
+    const FormatToken *FirstNonComment = TheLine->First;
+    if (FirstNonComment->is(tok::comment)) {
+      FirstNonComment = FirstNonComment->getNextNonComment();
+      if (!FirstNonComment)
+        return 0;
+    }
+    // FIXME: There are probably cases where we should use FirstNonComment
+    // instead of TheLine->First.
+
     if (Style.CompactNamespaces) {
       if (auto nsToken = TheLine->First->getNamespaceToken()) {
         int i = 0;
@@ -358,9 +367,9 @@ class LineJoiner {
     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,
-                                TT_ForEachMacro)) {
+    if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
+        FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
+                                 TT_ForEachMacro)) {
       return Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;

diff  --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 0d315734bc951..40de29e58737f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -1520,6 +1520,36 @@ TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
 
 TEST_F(FormatTest, FormatShortBracedStatements) {
   FormatStyle AllowSimpleBracedStatements = getLLVMStyle();
+  EXPECT_EQ(AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine, false);
+  EXPECT_EQ(AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine,
+            FormatStyle::SIS_Never);
+  EXPECT_EQ(AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine, false);
+  EXPECT_EQ(AllowSimpleBracedStatements.BraceWrapping.AfterFunction, false);
+  verifyFormat("for (;;) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("/*comment*/ for (;;) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("BOOST_FOREACH (int v, vec) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("/*comment*/ BOOST_FOREACH (int v, vec) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("while (true) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("/*comment*/ while (true) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("if (true) {\n"
+               "  f();\n"
+               "}");
+  verifyFormat("/*comment*/ if (true) {\n"
+               "  f();\n"
+               "}");
+
   AllowSimpleBracedStatements.IfMacros.push_back("MYIF");
   // Where line-lengths matter, a 2-letter synonym that maintains line length.
   // Not IF to avoid any confusion that IF is somehow special.