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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
