Page MenuHomePhabricator

[clang-format] Fix short block when braking after control statement
AcceptedPublic

Authored by Bouska on Dec 14 2019, 6:04 AM.

Details

Summary

This patch fixes bug #44192

When clang-format is run with option AllowShortBlocksOnASingleLine, it is expected to either succeed in putting the short block with its control statement on a single line or fail and leave the block as is. When brace wrapping after control statement is activated, if the block + the control statement length is superior to column limit but the block alone is not, clang-format puts the block in two lines: one for the control statement and one for the block. This patch removes this unexpected behaviour. Current unittests are updated to check for this behaviour.

Diff Detail

Event Timeline

Bouska created this revision.Dec 14 2019, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2019, 6:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
610

any reason you are changing existing tests?

Bouska added inline comments.Dec 21 2019, 8:37 AM
clang/unittests/Format/FormatTest.cpp
610

According to the change that added this test (https://reviews.llvm.org/D37140?id=113074), this test was made to check that "the wrapping is forced by exceeding the column limit". This should be check at the limit condition when the control statement + the block exceed the column limit by a few characters. With the current test, the length of the block statement alone is sufficient to force the wrapping, that's why with the current code this test has the expected wrapping.

Even my change is incorrect, with the control statement, the curly braces, the spaces and the statement, it is way above the column limit of 40. I'm going to fix that. Alternatively, I just could add another test with the correct length.

Bouska updated this revision to Diff 235435.Dec 27 2019, 10:24 AM

Set line length to column limit + 1 (41) in tests

Bouska updated this revision to Diff 236188.Jan 4 2020, 10:00 AM

Updated unittest to check under the column limit and over the column limit.

MyDeveloperDay requested changes to this revision.Jan 20 2020, 7:22 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
366

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.

clang/unittests/Format/FormatTest.cpp
686

I'm just not comfortable with changing tests, code can still be written in the way it was before, and I cannot tell what it would now be, I believe tests should be additive

This revision now requires changes to proceed.Jan 20 2020, 7:22 AM
Bouska marked an inline comment as done.Jan 20 2020, 9:04 AM
Bouska added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
366

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().

Bouska marked an inline comment as done.Jan 20 2020, 9:51 AM
Bouska added inline comments.
clang/unittests/Format/FormatTest.cpp
686

Ok, I'll keep the original test and add two new ones.

Bouska updated this revision to Diff 253366.Mar 28 2020, 11:29 AM

Do not touch current tests.

Bouska marked 3 inline comments as done.Mar 28 2020, 11:29 AM
MyDeveloperDay accepted this revision.Apr 6 2020, 11:36 AM

This seems to LGTM, in the absence of any other input, I guess we have to rely on the tests.

This revision is now accepted and ready to land.Apr 6 2020, 11:36 AM