r312904 - [clang-format] Fixed one-line if statement
Krasimir Georgiev via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 11 03:12:16 PDT 2017
Author: krasimir
Date: Mon Sep 11 03:12:16 2017
New Revision: 312904
URL: http://llvm.org/viewvc/llvm-project?rev=312904&view=rev
Log:
[clang-format] Fixed one-line if statement
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();
}
```
Contributed by @PriMee!
Reviewers: krasimir, djasper
Reviewed By: krasimir
Subscribers: cfe-commits, klimek
Differential Revision: https://reviews.llvm.org/D37140
Modified:
cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
cfe/trunk/unittests/Format/FormatTest.cpp
Modified: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp?rev=312904&r1=312903&r2=312904&view=diff
==============================================================================
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp (original)
+++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Mon Sep 11 03:12:16 2017
@@ -279,15 +279,43 @@ private:
}
}
+ // Try to merge a function block with left brace unwrapped
if (TheLine->Last->is(TT_FunctionLBrace) &&
TheLine->First != TheLine->Last) {
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
+ // Try to merge a control statement block with left brace unwrapped
+ 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;
+ }
+ // Try to merge a control statement block with left brace wrapped
+ 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;
+ }
+ // Try to merge either empty or one-line block if is precedeed by control
+ // statement token
+ 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;
+ }
+ // Try to merge a block with left brace wrapped that wasn't yet covered
if (TheLine->Last->is(tok::l_brace)) {
- return !Style.BraceWrapping.AfterFunction
+ return !Style.BraceWrapping.AfterFunction ||
+ (I[1]->First->is(tok::r_brace) &&
+ !Style.BraceWrapping.SplitEmptyRecord)
? tryMergeSimpleBlock(I, E, Limit)
: 0;
}
+ // Try to merge a function block with left brace wrapped
if (I[1]->First->is(TT_FunctionLBrace) &&
Style.BraceWrapping.AfterFunction) {
if (I[1]->Last->is(TT_LineComment))
@@ -437,18 +465,35 @@ private:
// Check that the current line allows merging. This depends on whether we
// are in a control flow statements as well as several style flags.
if (Line.First->isOneOf(tok::kw_else, tok::kw_case) ||
- (Line.First->Next && Line.First->Next->is(tok::kw_else)))
+ (Line.First->Next && Line.First->Next->is(tok::kw_else)) ||
+ (I != AnnotatedLines.begin() && I[-1]->Last->is(tok::kw_else)))
return 0;
if (Line.First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_do, tok::kw_try,
tok::kw___try, tok::kw_catch, tok::kw___finally,
tok::kw_for, tok::r_brace, Keywords.kw___except)) {
if (!Style.AllowShortBlocksOnASingleLine)
return 0;
+ // Don't merge when we can't except the case when
+ // the control statement block is empty
+ if (!Style.AllowShortIfStatementsOnASingleLine &&
+ Line.startsWith(tok::kw_if) &&
+ !Style.BraceWrapping.AfterControlStatement &&
+ !I[1]->First->is(tok::r_brace))
+ return 0;
if (!Style.AllowShortIfStatementsOnASingleLine &&
- Line.startsWith(tok::kw_if))
+ Line.startsWith(tok::kw_if) &&
+ Style.BraceWrapping.AfterControlStatement && I + 2 != E &&
+ !I[2]->First->is(tok::r_brace))
return 0;
if (!Style.AllowShortLoopsOnASingleLine &&
- Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for))
+ Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
+ !Style.BraceWrapping.AfterControlStatement &&
+ !I[1]->First->is(tok::r_brace))
+ return 0;
+ if (!Style.AllowShortLoopsOnASingleLine &&
+ Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for) &&
+ Style.BraceWrapping.AfterControlStatement && I + 2 != E &&
+ !I[2]->First->is(tok::r_brace))
return 0;
// FIXME: Consider an option to allow short exception handling clauses on
// a single line.
@@ -460,52 +505,73 @@ private:
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);
+ 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;
- if (!nextTwoLinesFitInto(I, Limit))
- 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);
- // 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)
+ if (!nextTwoLinesFitInto(I, Limit))
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;
+ // 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);
- // Don't merge "if (a) { .. } else {".
- if (Tok->Next && Tok->Next->is(tok::kw_else))
+ // 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;
+
+ 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;
}
Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=312904&r1=312903&r2=312904&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Sep 11 03:12:16 2017
@@ -439,11 +439,16 @@ TEST_F(FormatTest, FormatLoopsWithoutCom
TEST_F(FormatTest, FormatShortBracedStatements) {
FormatStyle AllowSimpleBracedStatements = getLLVMStyle();
+ AllowSimpleBracedStatements.ColumnLimit = 40;
AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
+ AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
+ AllowSimpleBracedStatements.BraceWrapping.AfterFunction = true;
+ AllowSimpleBracedStatements.BraceWrapping.SplitEmptyRecord = false;
+
verifyFormat("if (true) {}", AllowSimpleBracedStatements);
verifyFormat("if constexpr (true) {}", AllowSimpleBracedStatements);
verifyFormat("while (true) {}", AllowSimpleBracedStatements);
@@ -452,6 +457,10 @@ TEST_F(FormatTest, FormatShortBracedStat
verifyFormat("if constexpr (true) { f(); }", AllowSimpleBracedStatements);
verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+ verifyFormat("if (true) {\n"
+ " ffffffffffffffffffffffffffffffffffffffffffffffffffffff();\n"
+ "}",
+ AllowSimpleBracedStatements);
verifyFormat("if (true) { //\n"
" f();\n"
"}",
@@ -482,6 +491,7 @@ TEST_F(FormatTest, FormatShortBracedStat
AllowSimpleBracedStatements);
AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+ verifyFormat("if (true) {}", AllowSimpleBracedStatements);
verifyFormat("if (true) {\n"
" f();\n"
"}",
@@ -494,14 +504,83 @@ TEST_F(FormatTest, FormatShortBracedStat
AllowSimpleBracedStatements);
AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = false;
+ verifyFormat("while (true) {}", AllowSimpleBracedStatements);
verifyFormat("while (true) {\n"
" f();\n"
"}",
AllowSimpleBracedStatements);
+ verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
verifyFormat("for (;;) {\n"
" f();\n"
"}",
AllowSimpleBracedStatements);
+
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+ AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
+ AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
+
+ verifyFormat("if (true) {}", AllowSimpleBracedStatements);
+ verifyFormat("if constexpr (true) {}", AllowSimpleBracedStatements);
+ verifyFormat("while (true) {}", AllowSimpleBracedStatements);
+ verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
+ verifyFormat("if (true) { f(); }", AllowSimpleBracedStatements);
+ verifyFormat("if constexpr (true) { f(); }", AllowSimpleBracedStatements);
+ verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
+ verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{\n"
+ " ffffffffffffffffffffffffffffffffffffffffffffffffffffff();\n"
+ "}",
+ AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{ //\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{\n"
+ " f();\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{\n"
+ " f();\n"
+ "} else\n"
+ "{\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+ verifyFormat("if (true) {}", AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+ verifyFormat("if (true)\n"
+ "{\n"
+ " f();\n"
+ "} else\n"
+ "{\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+
+ AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = false;
+ verifyFormat("while (true) {}", AllowSimpleBracedStatements);
+ verifyFormat("while (true)\n"
+ "{\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
+ verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
+ verifyFormat("for (;;)\n"
+ "{\n"
+ " f();\n"
+ "}",
+ AllowSimpleBracedStatements);
}
TEST_F(FormatTest, ParseIfElse) {
More information about the cfe-commits
mailing list