[clang] [clang-format] Fix short function merging with macro expansion (PR #204421)
Jesse Rosenstock via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 19 00:29:12 PDT 2026
https://github.com/jmr updated https://github.com/llvm/llvm-project/pull/204421
>From 5aadc828299075b763880fb3a70b3611e25100f5 Mon Sep 17 00:00:00 2001
From: Jesse Rosenstock <jmr at google.com>
Date: Wed, 17 Jun 2026 16:02:46 +0200
Subject: [PATCH 1/3] [clang-format][NFC] Add tests for short function merging
with macro expansion
Add tests documenting the current (broken) behavior when macro expansion
produces control flow or multiple statements inside a short function body.
The formatter produces an inconsistent `{ code\n}` layout instead of
merging onto a single line or properly expanding to multiple lines.
Tests use LLVM style with explicit Macros config, independent of any
predefined style macros. Also add test coverage for the brace-in-body
readability check and the braced-init exception.
Assisted-by: Gemini
---
.../Format/FormatTestMacroExpansion.cpp | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index d391fe3d715c3..b6a8ff32aa3be 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -301,6 +301,68 @@ TEST_F(FormatTestMacroExpansion, IndentChildrenWithinMacroCall) {
Style);
}
+// Short function merging works when the macro expansion is a simple expression.
+TEST_F(FormatTestMacroExpansion, ShortFunctionMergingSimpleMacro) {
+ FormatStyle Style = getLLVMStyle();
+ Style.Macros.push_back("EXPR(x)=x");
+ verifyFormat("void f() { EXPR(x); }", Style);
+}
+
+// When the macro expansion contains control flow, short function merging
+// breaks: the opening brace stays on the function line, but the closing brace
+// gets its own line, producing the inconsistent `{ code\n}` layout.
+//
+// FIXME: The correct output is `void f() { IF_ERROR_RETURN(x); }`.
+TEST_F(FormatTestMacroExpansion, ShortFunctionMergingControlFlowMacro) {
+ FormatStyle Style = getLLVMStyle();
+ Style.Macros.push_back("IF_ERROR_RETURN(x)=if (x) return x");
+ verifyFormat("void f() { IF_ERROR_RETURN(x);\n}", Style);
+}
+
+// Same bug with a multi-statement macro containing control flow.
+//
+// FIXME: The correct output is:
+// `void f() { ASSIGN_OR_RETURN(v, F(), R()); }`.
+TEST_F(FormatTestMacroExpansion, ShortFunctionMergingMultiStmtMacro) {
+ FormatStyle Style = getLLVMStyle();
+ Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)");
+ Style.Macros.push_back("ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ // 2-arg version has no control flow -- merges fine.
+ verifyFormat("void f() { ASSIGN_OR_RETURN(v, F()); }", Style);
+ // 3-arg version has control flow -- broken.
+ verifyFormat("void f() { ASSIGN_OR_RETURN(v, F(), R());\n}", Style);
+}
+
+// When the function body is too long to fit on one line, the macro should
+// expand to a properly indented multi-line body (not the `{ code\n}` layout).
+TEST_F(FormatTestMacroExpansion, LongBodyWithMacroDoesNotMerge) {
+ FormatStyle Style = getLLVMStyleWithColumns(40);
+ Style.Macros.push_back("IF_ERROR_RETURN(x)=if (x) return x");
+ verifyFormat("void f() {\n"
+ " IF_ERROR_RETURN(long_function());\n"
+ "}",
+ Style);
+}
+
+// Function bodies containing braces (other than braced init) should not merge,
+// because `void f() { if (x) { return y; } }` is hard to read on one line.
+TEST_F(FormatTestMacroExpansion, BracesInBodyPreventMerging) {
+ FormatStyle Style = getLLVMStyle();
+ verifyFormat("void f() {\n"
+ " if (x) {\n"
+ " return y;\n"
+ " }\n"
+ "}",
+ Style);
+}
+
+// Braced init lists are excluded from the brace check -- they should still
+// allow short function merging.
+TEST_F(FormatTestMacroExpansion, BracedInitDoesNotPreventMerging) {
+ FormatStyle Style = getLLVMStyle();
+ verifyFormat("void f() { S s = {1}; }", Style);
+}
+
} // namespace
} // namespace test
} // namespace format
>From 7aad5363388b1761be33d3115238f6ab8c9d8e39 Mon Sep 17 00:00:00 2001
From: Jesse Rosenstock <jmr at google.com>
Date: Wed, 17 Jun 2026 11:41:33 +0200
Subject: [PATCH 2/3] [clang-format] Fix short function merging with macro
expansion
When a configured macro expands to multiple unwrapped lines (e.g.,
RETURN_IF_ERROR(x) expands to `if (x) return x`, producing separate
lines for `if` and `return`), tryMergeSimpleBlock failed to merge the
function body because it expected exactly three lines: { / body / }.
Scan forward past macro-expanded body lines (identified by MacroCtx on
their first token) to find the closing `}`, then check that all lines
fit within the column limit. This produces `void f() { MACRO(); }`
instead of the broken `void f() { MACRO();\n}` layout.
Assisted-by: Gemini
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 54 ++++++++++++++-----
.../Format/FormatTestMacroExpansion.cpp | 20 +++----
2 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 42eabc065b1a8..5c2d0c59964a5 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -975,28 +975,54 @@ class LineJoiner {
return 0;
}
- // Check that we still have three lines and they fit into the limit.
+ // We need at least three lines to merge: { / body / }.
if (I + 2 == E || I[2]->Type == LT_Invalid)
return 0;
- Limit = limitConsideringMacros(I + 2, E, Limit);
- if (!nextTwoLinesFitInto(I, Limit))
+ // When macro expansion produces additional unwrapped lines, scan
+ // forward past the expanded lines to find the closing `}`.
+ // For example, RETURN_IF_ERROR(x) expands to `if (x) return expr`;
+ // the parser creates separate unwrapped lines for the `if` and the
+ // `return`, so the function body has two lines instead of one.
+ unsigned NumBodyLines = 1;
+ if (I[1]->First->MacroCtx) {
+ while (I + NumBodyLines + 1 != E &&
+ I[NumBodyLines + 1]->First->MacroCtx &&
+ I[NumBodyLines + 1]->First->isNot(tok::r_brace)) {
+ ++NumBodyLines;
+ }
+ // Ran off the end without finding a closing `}`.
+ if (I + NumBodyLines + 1 == E)
+ return 0;
+ }
+
+ if (I[NumBodyLines + 1]->Type == LT_Invalid)
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))
+ // Check that all body lines plus the closing `}` fit.
+ Limit = limitConsideringMacros(I + NumBodyLines + 1, E, Limit);
+ // Range: opening line, NumBodyLines body lines, closing `}`.
+ if (!nextNLinesFitInto(I, I + NumBodyLines + 2, Limit))
return 0;
- do {
- if (Tok->isOneOf(tok::l_brace, tok::r_brace) &&
- Tok->isNot(BK_BracedInit)) {
+
+ // Check that none of the body lines end with a line comment or
+ // contain braces; nesting braces on a single line hurts readability.
+ // (For macro-expanded lines this is unlikely since brace-delimited
+ // blocks become child unwrapped lines, but check anyway.)
+ for (unsigned J = 0; J < NumBodyLines; ++J) {
+ const AnnotatedLine *BodyLine = I[J + 1];
+ if (BodyLine->Last->is(TT_LineComment))
return 0;
+ for (const FormatToken *T = BodyLine->First; T; T = T->Next) {
+ if (T->isOneOf(tok::l_brace, tok::r_brace) &&
+ T->isNot(BK_BracedInit)) {
+ return 0;
+ }
}
- Tok = Tok->Next;
- } while (Tok);
+ }
- // Last, check that the third line starts with a closing brace.
- Tok = I[2]->First;
+ // Check that the line after the body starts with a closing brace.
+ Tok = I[NumBodyLines + 1]->First;
if (Tok->isNot(tok::r_brace))
return 0;
@@ -1017,7 +1043,7 @@ class LineJoiner {
return 0;
}
- return 2;
+ return NumBodyLines + 1;
}
} else if (I[1]->First->is(tok::l_brace)) {
if (I[1]->Last->is(TT_LineComment))
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index b6a8ff32aa3be..735e852abcd6e 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -308,29 +308,23 @@ TEST_F(FormatTestMacroExpansion, ShortFunctionMergingSimpleMacro) {
verifyFormat("void f() { EXPR(x); }", Style);
}
-// When the macro expansion contains control flow, short function merging
-// breaks: the opening brace stays on the function line, but the closing brace
-// gets its own line, producing the inconsistent `{ code\n}` layout.
-//
-// FIXME: The correct output is `void f() { IF_ERROR_RETURN(x); }`.
+// Short function merging works when the macro expansion contains control flow.
TEST_F(FormatTestMacroExpansion, ShortFunctionMergingControlFlowMacro) {
FormatStyle Style = getLLVMStyle();
Style.Macros.push_back("IF_ERROR_RETURN(x)=if (x) return x");
- verifyFormat("void f() { IF_ERROR_RETURN(x);\n}", Style);
+ verifyFormat("void f() { IF_ERROR_RETURN(x); }", Style);
}
-// Same bug with a multi-statement macro containing control flow.
-//
-// FIXME: The correct output is:
-// `void f() { ASSIGN_OR_RETURN(v, F(), R()); }`.
+// Short function merging works with multi-statement macros containing
+// control flow.
TEST_F(FormatTestMacroExpansion, ShortFunctionMergingMultiStmtMacro) {
FormatStyle Style = getLLVMStyle();
Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)");
Style.Macros.push_back("ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
- // 2-arg version has no control flow -- merges fine.
+ // 2-arg version has no control flow.
verifyFormat("void f() { ASSIGN_OR_RETURN(v, F()); }", Style);
- // 3-arg version has control flow -- broken.
- verifyFormat("void f() { ASSIGN_OR_RETURN(v, F(), R());\n}", Style);
+ // 3-arg version has control flow.
+ verifyFormat("void f() { ASSIGN_OR_RETURN(v, F(), R()); }", Style);
}
// When the function body is too long to fit on one line, the macro should
>From 91a913d39daf3d481f1143d478e250cb16b5edc3 Mon Sep 17 00:00:00 2001
From: Jesse Rosenstock <jmr at google.com>
Date: Fri, 19 Jun 2026 09:07:42 +0200
Subject: [PATCH 3/3] [clang-format][NFC] Move non-macro tests to
FormatTest.cpp
Move BracesInBodyPreventMerging and BracedInitDoesNotPreventMerging from
FormatTestMacroExpansion.cpp to FormatTest.cpp, since these tests do not
use macro expansion and belong with the other short-function merging
tests.
Requested in https://github.com/llvm/llvm-project/pull/204421#discussion_r3438604354
---
clang/unittests/Format/FormatTest.cpp | 19 +++++++++++++++++++
.../Format/FormatTestMacroExpansion.cpp | 19 -------------------
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 83e2c5b38ceaf..119f55b538112 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -15142,6 +15142,25 @@ TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {
"}");
}
+// Function bodies containing braces (other than braced init) should not merge,
+// because `void f() { if (x) { return y; } }` is hard to read on one line.
+TEST_F(FormatTest, BracesInBodyPreventMerging) {
+ FormatStyle Style = getLLVMStyle();
+ verifyFormat("void f() {\n"
+ " if (x) {\n"
+ " return y;\n"
+ " }\n"
+ "}",
+ Style);
+}
+
+// Braced init lists are excluded from the brace check -- they should still
+// allow short function merging.
+TEST_F(FormatTest, BracedInitDoesNotPreventMerging) {
+ FormatStyle Style = getLLVMStyle();
+ verifyFormat("void f() { S s = {1}; }", Style);
+}
+
TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLine) {
FormatStyle MergeEmptyOnly = getLLVMStyle();
MergeEmptyOnly.AllowShortFunctionsOnASingleLine =
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index 735e852abcd6e..c6632c5ce5b01 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -338,25 +338,6 @@ TEST_F(FormatTestMacroExpansion, LongBodyWithMacroDoesNotMerge) {
Style);
}
-// Function bodies containing braces (other than braced init) should not merge,
-// because `void f() { if (x) { return y; } }` is hard to read on one line.
-TEST_F(FormatTestMacroExpansion, BracesInBodyPreventMerging) {
- FormatStyle Style = getLLVMStyle();
- verifyFormat("void f() {\n"
- " if (x) {\n"
- " return y;\n"
- " }\n"
- "}",
- Style);
-}
-
-// Braced init lists are excluded from the brace check -- they should still
-// allow short function merging.
-TEST_F(FormatTestMacroExpansion, BracedInitDoesNotPreventMerging) {
- FormatStyle Style = getLLVMStyle();
- verifyFormat("void f() { S s = {1}; }", Style);
-}
-
} // namespace
} // namespace test
} // namespace format
More information about the cfe-commits
mailing list