Bouska marked an inline comment as done.
Bouska added inline comments.
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:344
- return MergedLines;
- }
// Don't merge block with left brace wrapped after ObjC special blocks
----------------
MyDeveloperDay wrote:
> I just can't understand how we are free to just remove a chunk of code. Why
> is this not being some sort of
>
>
> ```
> if (Style.AllowShortBlocksOnASingleLine)
> ```
>
> You are just not giving a convincing argument that this is a safe change
> without us having to go in a debug the original problem ourselves.
This chunk of code only provides a bogus formatting and nothing else. So in
order to fix the bug, I need to remove the whole block of code.
I wasn't clear on why this chunk of code created the bug, so let me explained.
Let's take an example:
```
if (true)
{
doStuff();
}
```
with //Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always//
and //Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never//.
When formatting the first line, the control statement @ line 308 is going to
fail (because //Style.BraceWrapping.AfterControlStatement !=
FormatStyle::BWACS_Always//) but the control statement @ line 322 is going to
pass (because the second line is a left brace and the first token of the line
is a //if//). As //Style.BraceWrapping.AfterControlStatement ==
FormatStyle::BWACS_Always//, //tryMergeSimpleBlock()// is going to handle the
merging. There is two possible outcomes : either the block is simple and not
too long and it can be merge in one line, or it is not and the whole block
stays as it is. That line is then considered formated (due to the //return//).
When formatting the second line, the control statement @ line 332 (because the
first and only token of the line is a left brace and the first token of the
first line is a //if//) is going to pass and
//Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never//, so we are
going to redo //tryMergeSimpleBlock()// on the first line which failed the
first time! So this is the part I understand, we are doing someting we already
did. The part I don't understand is that this time //tryMergeSimpleBlock()//
managed to merge the line (probably because of some changes on the annoted
first line). As the merging worked, //MergedLines > 0// @ line 340 is true so
//MergedLines// is decremented (as explained by the comment) and we end up with
the block merged but without the control statement:
```
if (true)
{ doStuff(); }
```
Which is not the expected formating.
I think there might be another bug hidden somewhere in
//tryMergeSimpleBlock()//.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71512/new/
https://reviews.llvm.org/D71512
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
