This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fixed one-line if statement
ClosedPublic

Authored by PriMee on Aug 25 2017, 5:42 AM.

Details

Summary

Short overview:

Fixed bug: https://bugs.llvm.org/show_bug.cgi?id=34001
Clang-format bug resulting in a strange behavior of control statements short blocks. Different flags combinations do not guarantee expected result. Turned on option AllowShortBlocksOnASingleLine does not work as intended.

Description of the problem:

Cpp source file UnwrappedLineFormatter does not handle AllowShortBlocksOnASingleLine flag as it should. Putting a single-line control statement without any braces, clang-format works as expected (depending on AllowShortIfStatementOnASingleLine or AllowShortLoopsOnASingleLine value). Putting a single-line control statement in braces, we can observe strange and incorrect behavior.
Our short block is intercepted by tryFitMultipleLinesInOne function. The function returns a number of lines to be merged. Unfortunately, our control statement block is not covered properly. There are several if-return statements, but none of them handles our block. A block is identified by the line first token and by left and right braces. A function block works as expected, there is such an if-return statement doing proper job. A control statement block, from the other hand, falls into strange conditional construct, which depends on BraceWrapping.AfterFunction flag (with condition that the line’s last token is left brace, what is possible in our case) or goes even further. That should definitely not happen.

Description of the patch:

By adding three different if statements, we guarantee that our short control statement block, however it looks like (different brace wrapping flags may be turned on), is handled properly and does not fall into wrong conditional construct. Depending on appropriate options we return either 0 (when something disturbs our merging attempt) or let another function (tryMergeSimpleBlock) take the responsibility of returned result (number of merged lines). Nevertheless, one more correction is required in mentioned tryMergeSimpleBlock function. The function, previously, returned either 0 or 2. The problem was that this did not handle the case when our block had the left brace in a separate line, not the header one. After change, after adding condition, we return the result compatible with block’s structure. In case of left brace in the header’s line we do everything as before the patch. In case of left brace in a separate line we do the job similar to the one we do in case of a “non-header left brace” function short block. To be precise, we try to merge the block ignoring the header line. Then, if success, we increment our returned result.

After fix:

CONFIG:

AllowShortBlocksOnASingleLine: true 
AllowShortIfStatementsOnASingleLine: true 
BreakBeforeBraces: Custom
BraceWrapping: { 
AfterClass: true, AfterControlStatement: true, AfterEnum: true, AfterFunction: true, AfterNamespace: false, AfterStruct: true, AfterUnion: true, BeforeCatch: true, BeforeElse: true 
}

BEFORE:

if (statement) doSomething();
if (statement) { doSomething(); }
if (statement) {
    doSomething();
}
if (statement)
{
    doSomething();
}
if (statement)
    doSomething();
if (statement) {
    doSomething1();
    doSomething2();
}

AFTER:

if (statement) doSomething();
if (statement) { doSomething(); }
if (statement) { doSomething(); }
if (statement) { doSomething(); }
if (statement) doSomething();
if (statement)
{
  doSomething1();
  doSomething2();
}

Diff Detail

Repository
rL LLVM

Event Timeline

PriMee created this revision.Aug 25 2017, 5:42 AM
PriMee updated this revision to Diff 113074.Aug 29 2017, 6:11 AM

Unit tests added.

krasimir added inline comments.Aug 30 2017, 6:12 AM
lib/Format/UnwrappedLineFormatter.cpp
286 ↗(On Diff #113074)

No tests fail if this if statement gets removed. Please either remove it or add a test for it.

291 ↗(On Diff #113074)

Please add a short comment about which case is this addressing.

295 ↗(On Diff #113074)

Which case does this if cover?

296 ↗(On Diff #113074)

Please remove unnecessary parenthesis.

300 ↗(On Diff #113074)

Please reformat the newly added code with clang-format.

300 ↗(On Diff #113074)

Why in this case we use Style.AllowShortBlocksOnASingleLine and in the case above we use AfterControlStatement?
Also, why tryMergeSimpleBlock instead of tryMergeControlStatement?

538 ↗(On Diff #113074)

Please remove unnecessary parenthesis.

539 ↗(On Diff #113074)

Please reformat this code with clang-format. This block must be indented.

unittests/Format/FormatTest.cpp
519 ↗(On Diff #113074)

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();
}
537 ↗(On Diff #113074)

Why is this example not on a single line? Is it because it has an else?

539 ↗(On Diff #113074)

Could you please also add a test with empty braces, like if (true) {} after this?

krasimir edited edge metadata.Aug 30 2017, 6:26 AM
PriMee updated this revision to Diff 113379.Aug 31 2017, 3:18 AM

Diff file updated. Some tests added. Some new bugs fixed as well :)

Sorry for wrong formatting before. Some inline comments added.

lib/Format/UnwrappedLineFormatter.cpp
286 ↗(On Diff #113074)

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.

300 ↗(On Diff #113074)

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.

unittests/Format/FormatTest.cpp
537 ↗(On Diff #113074)

Exactly. Shouldn't this work like that? There is a comment:
// Don't merge "if (a) { .. } else {". on line 548 in UnwrappedLineFormatter.cpp

PriMee updated this revision to Diff 113979.Sep 6 2017, 3:26 AM

Diff file again updated. Created against the newest commit.

PriMee added inline comments.Sep 6 2017, 3:31 AM
lib/Format/UnwrappedLineFormatter.cpp
286 ↗(On Diff #113074)

Okay, done. Now the test case would fail (Line 449 in FormatTest.cpp would be problematic, although in our case should NOT change anything).

krasimir accepted this revision.Sep 8 2017, 7:13 AM

Great! Would you like me to commit this for you?

This revision is now accepted and ready to land.Sep 8 2017, 7:13 AM
PriMee added a comment.Sep 8 2017, 7:40 AM

Yes, would be great :) Thank you!

This revision was automatically updated to reflect the committed changes.