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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
