From e220825c7db1b2acdcf048811de17b07ba6e3a0e Mon Sep 17 00:00:00 2001 From: sstwcw Date: Fri, 3 Oct 2025 20:22:20 +0000 Subject: [PATCH 1/3] [clang-format] Continue aligned lines better after with config `{AlignConsecutiveAssignments: true}` ```C++ auto aaaaaaaaaaaaaaaaaaaaa = {}; auto b = [] { x = {.one_foooooooooooooooo = 2, // .two_fooooooooooooo = 3, // .three_fooooooooooooo = 4}; }; A B = {"Hello " "World"}; BYTE payload = 2; float i2 = 0; auto v = type{i2 = 1, // i = 3}; ``` before ```C++ auto aaaaaaaaaaaaaaaaaaaaa = {}; auto b = [] { x = {.one_foooooooooooooooo = 2, // .two_fooooooooooooo = 3, // .three_fooooooooooooo = 4}; }; A B = {"Hello " "World"}; BYTE payload = 2; float i2 = 0; auto v = type{i2 = 1, // i = 3}; ``` When a line gets aligned, the following lines may need to move with it. This patch replaces the old algorithm with a simpler one. It uses the `IsAligned` attribute. It makes most of the scope stack irrelevant. Now the stack only needs to keep track of 2 levels. The old algorithm had problems like these. - Whether lines inside a braced list moved depended on whether there was a type at the start. It should depend on whether the inside was aligned to the brace. The first case that came up with a type name at the start happened to have a comma at the end of the list so the inside was not aligned to the brace. - Excluding lines inside closures did not always work. - A continued string could move twice as much as it should. The following problems are not fixed yet. A token that opens a scope is needed. Operator precedence is not enough. ```C++ auto aaaaaaaaaaaaaaaaaaaaa = {}; auto b = 0 + // 0; ``` The algorithm has trouble when things that should not move and things that should move are nested. This is related to the `IsAligned` attribute being a boolean. It also affects how spaces and tabs are selected. ```C++ auto aaaaaaaaaaaaaaaaaaaaa = {}; auto b = {.a = { .a = 0, }}; ``` --- clang/lib/Format/WhitespaceManager.cpp | 128 +++++-------------------- clang/unittests/Format/FormatTest.cpp | 48 ++++++++++ 2 files changed, 74 insertions(+), 102 deletions(-) diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index 54f366fc02502..8e2f8cc8cdd4a 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -289,17 +289,20 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, SmallVector &Changes) { int Shift = 0; - // ScopeStack keeps track of the current scope depth. It contains indices of - // the first token on each scope. + // ScopeStack keeps track of the current scope depth. It contains the levels + // of at most 2 scopes. The first one is the one that the matched token is + // in. The second one is the one that should not be moved by this procedure. // The "Matches" indices should only have tokens from the outer-most scope. // However, we do need to pay special attention to one class of tokens - // that are not in the outer-most scope, and that is function parameters - // which are split across multiple lines, as illustrated by this example: + // that are not in the outer-most scope, and that is the continuations of an + // unwrapped line whose positions are derived from a token to the right of the + // aligned token, as illustrated by this example: // double a(int x); // int b(int y, // double z); // In the above example, we need to take special care to ensure that - // 'double z' is indented along with it's owning function 'b'. + // 'double z' is indented along with its owning function 'b', because its + // position is derived from the '(' token to the right of the 'b' token. // The same holds for calling a function: // double a = foo(x); // int b = bar(foo(y), @@ -309,32 +312,28 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, // auto s = "Hello" // "World"; // Special handling is required for 'nested' ternary operators. - SmallVector ScopeStack; + SmallVector, 2> ScopeStack; for (unsigned i = Start; i != End; ++i) { auto &CurrentChange = Changes[i]; if (!Matches.empty() && Matches[0] < i) Matches.consume_front(); assert(Matches.empty() || Matches[0] >= i); - if (!ScopeStack.empty() && - CurrentChange.indentAndNestingLevel() < - Changes[ScopeStack.back()].indentAndNestingLevel()) { + while (!ScopeStack.empty() && + CurrentChange.indentAndNestingLevel() < ScopeStack.back()) { ScopeStack.pop_back(); } - // Compare current token to previous non-comment token to ensure whether - // it is in a deeper scope or not. - unsigned PreviousNonComment = i - 1; - while (PreviousNonComment > Start && - Changes[PreviousNonComment].Tok->is(tok::comment)) { - --PreviousNonComment; - } - if (i != Start && CurrentChange.indentAndNestingLevel() > - Changes[PreviousNonComment].indentAndNestingLevel()) { - ScopeStack.push_back(i); + // Keep track of the level that should not move with the aligned token. + if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore != 0u && + CurrentChange.indentAndNestingLevel() > ScopeStack[0] && + !CurrentChange.IsAligned) { + ScopeStack.push_back(CurrentChange.indentAndNestingLevel()); } - bool InsideNestedScope = !ScopeStack.empty(); + bool InsideNestedScope = + !ScopeStack.empty() && + CurrentChange.indentAndNestingLevel() > ScopeStack[0]; bool ContinuedStringLiteral = i > Start && CurrentChange.Tok->is(tok::string_literal) && Changes[i - 1].Tok->is(tok::string_literal); @@ -349,94 +348,22 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, if (!Matches.empty() && Matches[0] == i) { Shift = Column - (RightJustify ? CurrentChange.TokenLength : 0) - CurrentChange.StartOfTokenColumn; + ScopeStack = {CurrentChange.indentAndNestingLevel()}; CurrentChange.Spaces += Shift; } if (Shift == 0) continue; - // This is for function parameters that are split across multiple lines, - // as mentioned in the ScopeStack comment. - if (InsideNestedScope && CurrentChange.NewlinesBefore > 0) { - unsigned ScopeStart = ScopeStack.back(); + // This is for lines that are split across multiple lines, as mentioned in + // the ScopeStack comment. The stack size being 1 means that the token is + // not in a scope that should not move. + if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore > 0) { auto ShouldShiftBeAdded = [&] { - // Function declaration - if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName)) - return true; - - // Lambda. - if (Changes[ScopeStart - 1].Tok->is(TT_LambdaLBrace)) - return false; - - // Continued function declaration - if (ScopeStart > Start + 1 && - Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)) { - return true; - } - - // Continued (template) function call. - if (ScopeStart > Start + 1 && - Changes[ScopeStart - 2].Tok->isOneOf(tok::identifier, - TT_TemplateCloser) && - Changes[ScopeStart - 1].Tok->is(tok::l_paren) && - Changes[ScopeStart].Tok->isNot(TT_LambdaLSquare)) { - if (CurrentChange.Tok->MatchingParen && - CurrentChange.Tok->MatchingParen->is(TT_LambdaLBrace)) { - return false; - } - if (Changes[ScopeStart].NewlinesBefore > 0) - return false; - if (CurrentChange.Tok->is(tok::l_brace) && - CurrentChange.Tok->is(BK_BracedInit)) { - return true; - } - return Style.BinPackArguments; - } - - // Ternary operator - if (CurrentChange.Tok->is(TT_ConditionalExpr)) - return true; - - // Period Initializer .XXX = 1. - if (CurrentChange.Tok->is(TT_DesignatedInitializerPeriod)) - return true; - - // Continued ternary operator - if (CurrentChange.Tok->Previous && - CurrentChange.Tok->Previous->is(TT_ConditionalExpr)) { - return true; - } - - // Continued direct-list-initialization using braced list. - if (ScopeStart > Start + 1 && - Changes[ScopeStart - 2].Tok->is(tok::identifier) && - Changes[ScopeStart - 1].Tok->is(tok::l_brace) && - CurrentChange.Tok->is(tok::l_brace) && - CurrentChange.Tok->is(BK_BracedInit)) { - return true; - } - - // Continued braced list. - if (ScopeStart > Start + 1 && - Changes[ScopeStart - 2].Tok->isNot(tok::identifier) && - Changes[ScopeStart - 1].Tok->is(tok::l_brace) && - CurrentChange.Tok->isNot(tok::r_brace)) { - for (unsigned OuterScopeStart : llvm::reverse(ScopeStack)) { - // Lambda. - if (OuterScopeStart > Start && - Changes[OuterScopeStart - 1].Tok->is(TT_LambdaLBrace)) { - return false; - } - } - if (Changes[ScopeStart].NewlinesBefore > 0) - return false; + if (ContinuedStringLiteral) return true; - } - - // Continued template parameter. - if (Changes[ScopeStart - 1].Tok->is(TT_TemplateOpener)) + if (InsideNestedScope && CurrentChange.IsAligned) return true; - return false; }; @@ -444,9 +371,6 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, CurrentChange.Spaces += Shift; } - if (ContinuedStringLiteral) - CurrentChange.Spaces += Shift; - // We should not remove required spaces unless we break the line before. assert(Shift > 0 || Changes[i].NewlinesBefore > 0 || CurrentChange.Spaces >= diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index fef70365b5e18..b3aebd50cf400 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -19575,6 +19575,12 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) { " return;\n" "};", Alignment); + verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n" + "auto b = g([] {\n" + " return \"Hello \"\n" + " \"World\";\n" + "});", + Alignment); verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n" "auto b = g([] {\n" " f();\n" @@ -20192,6 +20198,11 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) { " i = 3 //\n" "};", Alignment); + // When assignments are nested, each level should be aligned. + verifyFormat("float i2 = 0;\n" + "auto v = type{i2 = 1, //\n" + " i = 3};", + Alignment); Alignment.AlignConsecutiveAssignments.Enabled = false; verifyFormat( @@ -20681,6 +20692,21 @@ TEST_F(FormatTest, AlignWithLineBreaks) { "}", Style); + verifyFormat("void foo() {\n" + " int myVar = 5;\n" + " double x = 3.14;\n" + " auto str = (\"Hello \"\n" + " \"World\");\n" + " auto s = (\"Hello \"\n" + " \"Again\");\n" + "}", + Style); + + verifyFormat("A B = {\"Hello \"\n" + " \"World\"};\n" + "BYTE payload = 2;", + Style); + // clang-format off verifyFormat("void foo() {\n" " const int capacityBefore = Entries.capacity();\n" @@ -20763,6 +20789,28 @@ TEST_F(FormatTest, AlignWithInitializerPeriods) { "}", Style); + // The lines inside the braces are supposed to be indented by + // BracedInitializerIndentWidth from the start of the line. They should not + // move with the opening brace. + verifyFormat("void foo2(void) {\n" + " BYTE p[1] = 1;\n" + " A B = {\n" + " .one_foooooooooooooooo = 2,\n" + " .two_fooooooooooooo = 3,\n" + " .three_fooooooooooooo = 4,\n" + " };\n" + " BYTE payload = 2;\n" + "}", + Style); + + verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n" + "auto b = g([] {\n" + " x = {.one_foooooooooooooooo = 2, //\n" + " .two_fooooooooooooo = 3, //\n" + " .three_fooooooooooooo = 4};\n" + "});", + Style); + Style.AlignConsecutiveAssignments.Enabled = false; Style.AlignConsecutiveDeclarations.Enabled = true; verifyFormat("void foo3(void) {\n" From bfd79e7543bac34ed9b9f21407a5339b8d6444d7 Mon Sep 17 00:00:00 2001 From: sstwcw Date: Mon, 6 Oct 2025 21:30:11 +0000 Subject: [PATCH 2/3] Simplify --- clang/lib/Format/WhitespaceManager.cpp | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index 8e2f8cc8cdd4a..2462c49e98af7 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -358,17 +358,10 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, // This is for lines that are split across multiple lines, as mentioned in // the ScopeStack comment. The stack size being 1 means that the token is // not in a scope that should not move. - if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore > 0) { - auto ShouldShiftBeAdded = [&] { - if (ContinuedStringLiteral) - return true; - if (InsideNestedScope && CurrentChange.IsAligned) - return true; - return false; - }; - - if (ShouldShiftBeAdded()) - CurrentChange.Spaces += Shift; + if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore > 0 && + (ContinuedStringLiteral || + (InsideNestedScope && CurrentChange.IsAligned))) { + CurrentChange.Spaces += Shift; } // We should not remove required spaces unless we break the line before. From a63da275b4f3e341776ad38941cffbc5f7c38bc5 Mon Sep 17 00:00:00 2001 From: sstwcw Date: Thu, 9 Oct 2025 22:06:11 +0000 Subject: [PATCH 3/3] Simplify --- clang/lib/Format/WhitespaceManager.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index 2462c49e98af7..7348a3af8cf95 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -359,8 +359,7 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End, // the ScopeStack comment. The stack size being 1 means that the token is // not in a scope that should not move. if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore > 0 && - (ContinuedStringLiteral || - (InsideNestedScope && CurrentChange.IsAligned))) { + (ContinuedStringLiteral || InsideNestedScope)) { CurrentChange.Spaces += Shift; }