Skip to content

Commit 9cb9445

Browse files
committed
[clang-format] Correctly format loops and if statements even if preceded with comments.
Fixes llvm/llvm-project#53758. Braces in loops and in `if` statements with leading (block) comments were formatted according to `BraceWrapping.AfterFunction` and not `AllowShortBlocksOnASingleLine`/`AllowShortLoopsOnASingleLine`/`AllowShortIfStatementsOnASingleLine`. Previously, the code: ``` while (true) { f(); } /*comment*/ while (true) { f(); } ``` was incorrectly formatted to: ``` while (true) { f(); } /*comment*/ while (true) { f(); } ``` when using config: ``` BasedOnStyle: LLVM BreakBeforeBraces: Custom BraceWrapping: AfterFunction: false AllowShortBlocksOnASingleLine: false AllowShortLoopsOnASingleLine: false ``` and it was (correctly but by chance) formatted to: ``` while (true) { f(); } /*comment*/ while (true) { f(); } ``` when using enabling brace wrapping after functions: ``` BasedOnStyle: LLVM BreakBeforeBraces: Custom BraceWrapping: AfterFunction: true AllowShortBlocksOnASingleLine: false AllowShortLoopsOnASingleLine: false ``` Reviewed By: MyDeveloperDay, HazardyKnusperkeks, owenpan Differential Revision: https://reviews.llvm.org/D119649
1 parent 986afe8 commit 9cb9445

File tree

2 files changed

+42
-3
lines changed

2 files changed

+42
-3
lines changed

clang/lib/Format/UnwrappedLineFormatter.cpp

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,15 @@ class LineJoiner {
319319

320320
bool MergeShortFunctions = ShouldMergeShortFunctions();
321321

322+
const FormatToken *FirstNonComment = TheLine->First;
323+
if (FirstNonComment->is(tok::comment)) {
324+
FirstNonComment = FirstNonComment->getNextNonComment();
325+
if (!FirstNonComment)
326+
return 0;
327+
}
328+
// FIXME: There are probably cases where we should use FirstNonComment
329+
// instead of TheLine->First.
330+
322331
if (Style.CompactNamespaces) {
323332
if (auto nsToken = TheLine->First->getNamespaceToken()) {
324333
int i = 0;
@@ -358,9 +367,9 @@ class LineJoiner {
358367
if (TheLine->Last->is(TT_FunctionLBrace) && TheLine->First != TheLine->Last)
359368
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
360369
// Try to merge a control statement block with left brace unwrapped.
361-
if (TheLine->Last->is(tok::l_brace) && TheLine->First != TheLine->Last &&
362-
TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
363-
TT_ForEachMacro)) {
370+
if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
371+
FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
372+
TT_ForEachMacro)) {
364373
return Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never
365374
? tryMergeSimpleBlock(I, E, Limit)
366375
: 0;

clang/unittests/Format/FormatTest.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,6 +1520,36 @@ TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
15201520

15211521
TEST_F(FormatTest, FormatShortBracedStatements) {
15221522
FormatStyle AllowSimpleBracedStatements = getLLVMStyle();
1523+
EXPECT_EQ(AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine, false);
1524+
EXPECT_EQ(AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine,
1525+
FormatStyle::SIS_Never);
1526+
EXPECT_EQ(AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine, false);
1527+
EXPECT_EQ(AllowSimpleBracedStatements.BraceWrapping.AfterFunction, false);
1528+
verifyFormat("for (;;) {\n"
1529+
" f();\n"
1530+
"}");
1531+
verifyFormat("/*comment*/ for (;;) {\n"
1532+
" f();\n"
1533+
"}");
1534+
verifyFormat("BOOST_FOREACH (int v, vec) {\n"
1535+
" f();\n"
1536+
"}");
1537+
verifyFormat("/*comment*/ BOOST_FOREACH (int v, vec) {\n"
1538+
" f();\n"
1539+
"}");
1540+
verifyFormat("while (true) {\n"
1541+
" f();\n"
1542+
"}");
1543+
verifyFormat("/*comment*/ while (true) {\n"
1544+
" f();\n"
1545+
"}");
1546+
verifyFormat("if (true) {\n"
1547+
" f();\n"
1548+
"}");
1549+
verifyFormat("/*comment*/ if (true) {\n"
1550+
" f();\n"
1551+
"}");
1552+
15231553
AllowSimpleBracedStatements.IfMacros.push_back("MYIF");
15241554
// Where line-lengths matter, a 2-letter synonym that maintains line length.
15251555
// Not IF to avoid any confusion that IF is somehow special.

0 commit comments

Comments
 (0)