[PATCH] D71512: [clang-format] Fix short block when braking after control statement

[Pablo Martin-Gomez via Phabricator via cfe-commits]

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