PriMee added a comment.
Sorry for wrong formatting before. Some inline comments added.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:286
}
+ if (TheLine->Last->is(tok::l_brace) &&
+ TheLine->First != TheLine->Last &&
----------------
krasimir wrote:
> No tests fail if this `if` statement gets removed. Please either remove it or
> add a test for it.
That's true. Why is that? Without this if statement tests would pass because
our block would have been handled by the construct I have mentioned in problem
description (Line 311). Adding a very similar test case with
BraceWrapping.AfterFunction flag changed would show that we need this if
statement. To my mind, it doesn't make sense to add such test case.
================
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:
> 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`?
We use `AfterControlStatement` because we want to ensure that the block with
the opening brace in separate line is correct. The flag implies this fact.
We use `tryMergeControlStatement` only when our control statement is not within
braces, `tryMergeSimpleBlock` in every other case.
================
Comment at: unittests/Format/FormatTest.cpp:537
+ "}",
+ AllowSimpleBracedStatements);
+
----------------
krasimir wrote:
> Why is this example not on a single line? Is it because it has an `else`?
Exactly. Shouldn't this work like that? There is a comment:
`// Don't merge "if (a) { .. } else {".` on line 548 in
UnwrappedLineFormatter.cpp
https://reviews.llvm.org/D37140
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
