[PATCH] D37140: [clang-format] Fixed one-line if statement

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:286
     }
+    if (TheLine->Last->is(tok::l_brace) &&
+        TheLine->First != TheLine->Last &&
----------------
No tests fail if this `if` statement gets removed. Please either remove it or add a test for it.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:291
+    }
+    if (I[1]->First->is(tok::l_brace) && 
+        TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) { 
----------------
Please add a short comment about which case is this addressing.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:295
+    }
+    if (TheLine->First->is(tok::l_brace) &&
+       (TheLine->First == TheLine->Last) &&
----------------
Which case does this `if` cover?


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:296
+    if (TheLine->First->is(tok::l_brace) &&
+       (TheLine->First == TheLine->Last) &&
+       (I != AnnotatedLines.begin()) &&
----------------
Please remove unnecessary parenthesis.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:300
+      return Style.AllowShortBlocksOnASingleLine ? tryMergeSimpleBlock(I-1, E, Limit) : 0;
+    }
     if (TheLine->Last->is(tok::l_brace)) {
----------------
Please reformat the newly added code with `clang-format`.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:300
+      return Style.AllowShortBlocksOnASingleLine ? tryMergeSimpleBlock(I-1, E, Limit) : 0;
+    }
     if (TheLine->Last->is(tok::l_brace)) {
----------------
krasimir wrote:
> Please reformat the newly added code with `clang-format`.
Why in this case we use `Style.AllowShortBlocksOnASingleLine` and in the case above we use `AfterControlStatement`?
Also, why `tryMergeSimpleBlock` instead of `tryMergeControlStatement`?


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:538
+          (I[1]->First == I[1]->Last && I + 2 != E &&
+          I[2]->First->is(tok::r_brace)))) {
+      MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
----------------
Please remove unnecessary parenthesis.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:539
+          I[2]->First->is(tok::r_brace)))) {
+      MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
+      // If we managed to merge the block, count the statement header, which is
----------------
Please reformat this code with clang-format. This block must be indented.


================
Comment at: unittests/Format/FormatTest.cpp:519
+  verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n" 
+               "{ //\n"
----------------
Could you also please add a test case where the wrapping is forced by exceeding the column limit instead of by the empty comment, like:
```
if (true)
{
  ffffffffffffffffffffff();
}
```


================
Comment at: unittests/Format/FormatTest.cpp:537
+               "}",
+               AllowSimpleBracedStatements);
+
----------------
Why is this example not on a single line? Is it because it has an `else`?


================
Comment at: unittests/Format/FormatTest.cpp:539
+
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  verifyFormat("if (true)\n"
----------------
Could you please also add a test with empty braces, like `if (true) {}` after this?


https://reviews.llvm.org/D37140