PriMee created this revision.
Herald added a subscriber: klimek.
**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();
}
https://reviews.llvm.org/D37140
Files:
lib/Format/UnwrappedLineFormatter.cpp
Index: lib/Format/UnwrappedLineFormatter.cpp
===================================================================
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -283,6 +283,21 @@
TheLine->First != TheLine->Last) {
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
+ if (TheLine->Last->is(tok::l_brace) &&
+ TheLine->First != TheLine->Last &&
+ TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
+ return Style.AllowShortBlocksOnASingleLine ? tryMergeSimpleBlock(I, E, Limit) : 0;
+ }
+ if (I[1]->First->is(tok::l_brace) &&
+ TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
+ return Style.BraceWrapping.AfterControlStatement ? tryMergeSimpleBlock(I, E, Limit) : 0;
+ }
+ if (TheLine->First->is(tok::l_brace) &&
+ (TheLine->First == TheLine->Last) &&
+ (I != AnnotatedLines.begin()) &&
+ (I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for))) {
+ return Style.AllowShortBlocksOnASingleLine ? tryMergeSimpleBlock(I-1, E, Limit) : 0;
+ }
if (TheLine->Last->is(tok::l_brace)) {
return !Style.BraceWrapping.AfterFunction
? tryMergeSimpleBlock(I, E, Limit)
@@ -459,53 +474,74 @@
Keywords.kw___except, tok::kw___finally))
return 0;
}
+
+ if (Line.Last->is(tok::l_brace)) {
+ FormatToken *Tok = I[1]->First;
+ if (Tok->is(tok::r_brace) && !Tok->MustBreakBefore &&
+ (Tok->getNextNonComment() == nullptr ||
+ Tok->getNextNonComment()->is(tok::semi))) {
+ // We merge empty blocks even if the line exceeds the column limit.
+ Tok->SpacesRequiredBefore = 0;
+ Tok->CanBreakBefore = true;
+ return 1;
+ } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
+ !startsExternCBlock(Line)) {
+ // We don't merge short records.
+ FormatToken *RecordTok =
+ Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
+ if (RecordTok &&
+ RecordTok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
+ Keywords.kw_interface))
+ return 0;
- FormatToken *Tok = I[1]->First;
- if (Tok->is(tok::r_brace) && !Tok->MustBreakBefore &&
- (Tok->getNextNonComment() == nullptr ||
- Tok->getNextNonComment()->is(tok::semi))) {
- // We merge empty blocks even if the line exceeds the column limit.
- Tok->SpacesRequiredBefore = 0;
- Tok->CanBreakBefore = true;
- return 1;
- } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
- !startsExternCBlock(Line)) {
- // We don't merge short records.
- FormatToken *RecordTok =
- Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
- if (RecordTok &&
- RecordTok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
- Keywords.kw_interface))
- return 0;
+ // Check that we still have three lines and they fit into the limit.
+ if (I + 2 == E || I[2]->Type == LT_Invalid)
+ return 0;
+ Limit = limitConsideringMacros(I + 2, E, Limit);
- // Check that we still have three lines and they fit into the limit.
- if (I + 2 == E || I[2]->Type == LT_Invalid)
- return 0;
- Limit = limitConsideringMacros(I + 2, E, Limit);
+ if (!nextTwoLinesFitInto(I, Limit))
+ return 0;
- if (!nextTwoLinesFitInto(I, Limit))
- return 0;
+ // Second, check that the next line does not contain any braces - if it
+ // does, readability declines when putting it into a single line.
+ if (I[1]->Last->is(TT_LineComment))
+ return 0;
+ do {
+ if (Tok->is(tok::l_brace) && Tok->BlockKind != BK_BracedInit)
+ return 0;
+ Tok = Tok->Next;
+ } while (Tok);
- // Second, check that the next line does not contain any braces - if it
- // does, readability declines when putting it into a single line.
- if (I[1]->Last->is(TT_LineComment))
- return 0;
- do {
- if (Tok->is(tok::l_brace) && Tok->BlockKind != BK_BracedInit)
+ // Last, check that the third line starts with a closing brace.
+ Tok = I[2]->First;
+ if (Tok->isNot(tok::r_brace))
return 0;
- Tok = Tok->Next;
- } while (Tok);
- // Last, check that the third line starts with a closing brace.
- Tok = I[2]->First;
- if (Tok->isNot(tok::r_brace))
- return 0;
+ // Don't merge "if (a) { .. } else {".
+ if (Tok->Next && Tok->Next->is(tok::kw_else))
+ return 0;
- // Don't merge "if (a) { .. } else {".
- if (Tok->Next && Tok->Next->is(tok::kw_else))
+ return 2;
+ }
+ } else if (I[1]->First->is(tok::l_brace)) {
+ if (I[1]->Last->is(TT_LineComment))
return 0;
- return 2;
+ // Check for Limit <= 2 to account for the " {".
+ if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(*I)))
+ return 0;
+ Limit -= 2;
+ unsigned MergedLines = 0;
+ if ((Style.AllowShortBlocksOnASingleLine ||
+ (I[1]->First == I[1]->Last && I + 2 != E &&
+ I[2]->First->is(tok::r_brace)))) {
+ MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
+ // If we managed to merge the block, count the statement header, which is
+ // on a separate line.
+ if (MergedLines > 0)
+ ++MergedLines;
+ }
+ return MergedLines;
}
return 0;
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
