Skip to content

Commit a0b8261

Browse files
sstwcwsstwcw
authored
[clang-format] Continue aligned lines better (#161903)
Fixes #53497. Fixes #56078. 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, }}; ```
1 parent 7d3729d commit a0b8261

File tree

2 files changed

+88
-116
lines changed

2 files changed

+88
-116
lines changed

‎clang/lib/Format/WhitespaceManager.cpp‎

Lines changed: 26 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -289,17 +289,20 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
289289
SmallVector<WhitespaceManager::Change, 16> &Changes) {
290290
int Shift = 0;
291291

292-
// ScopeStack keeps track of the current scope depth. It contains indices of
293-
// the first token on each scope.
292+
// ScopeStack keeps track of the current scope depth. It contains the levels
293+
// of at most 2 scopes. The first one is the one that the matched token is
294+
// in. The second one is the one that should not be moved by this procedure.
294295
// The "Matches" indices should only have tokens from the outer-most scope.
295296
// However, we do need to pay special attention to one class of tokens
296-
// that are not in the outer-most scope, and that is function parameters
297-
// which are split across multiple lines, as illustrated by this example:
297+
// that are not in the outer-most scope, and that is the continuations of an
298+
// unwrapped line whose positions are derived from a token to the right of the
299+
// aligned token, as illustrated by this example:
298300
// double a(int x);
299301
// int b(int y,
300302
// double z);
301303
// In the above example, we need to take special care to ensure that
302-
// 'double z' is indented along with it's owning function 'b'.
304+
// 'double z' is indented along with its owning function 'b', because its
305+
// position is derived from the '(' token to the right of the 'b' token.
303306
// The same holds for calling a function:
304307
// double a = foo(x);
305308
// int b = bar(foo(y),
@@ -309,32 +312,28 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
309312
// auto s = "Hello"
310313
// "World";
311314
// Special handling is required for 'nested' ternary operators.
312-
SmallVector<unsigned, 16> ScopeStack;
315+
SmallVector<std::tuple<unsigned, unsigned, unsigned>, 2> ScopeStack;
313316

314317
for (unsigned i = Start; i != End; ++i) {
315318
auto &CurrentChange = Changes[i];
316319
if (!Matches.empty() && Matches[0] < i)
317320
Matches.consume_front();
318321
assert(Matches.empty() || Matches[0] >= i);
319-
if (!ScopeStack.empty() &&
320-
CurrentChange.indentAndNestingLevel() <
321-
Changes[ScopeStack.back()].indentAndNestingLevel()) {
322+
while (!ScopeStack.empty() &&
323+
CurrentChange.indentAndNestingLevel() < ScopeStack.back()) {
322324
ScopeStack.pop_back();
323325
}
324326

325-
// Compare current token to previous non-comment token to ensure whether
326-
// it is in a deeper scope or not.
327-
unsigned PreviousNonComment = i - 1;
328-
while (PreviousNonComment > Start &&
329-
Changes[PreviousNonComment].Tok->is(tok::comment)) {
330-
--PreviousNonComment;
331-
}
332-
if (i != Start && CurrentChange.indentAndNestingLevel() >
333-
Changes[PreviousNonComment].indentAndNestingLevel()) {
334-
ScopeStack.push_back(i);
327+
// Keep track of the level that should not move with the aligned token.
328+
if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore != 0u &&
329+
CurrentChange.indentAndNestingLevel() > ScopeStack[0] &&
330+
!CurrentChange.IsAligned) {
331+
ScopeStack.push_back(CurrentChange.indentAndNestingLevel());
335332
}
336333

337-
bool InsideNestedScope = !ScopeStack.empty();
334+
bool InsideNestedScope =
335+
!ScopeStack.empty() &&
336+
CurrentChange.indentAndNestingLevel() > ScopeStack[0];
338337
bool ContinuedStringLiteral = i > Start &&
339338
CurrentChange.Tok->is(tok::string_literal) &&
340339
Changes[i - 1].Tok->is(tok::string_literal);
@@ -349,103 +348,20 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
349348
if (!Matches.empty() && Matches[0] == i) {
350349
Shift = Column - (RightJustify ? CurrentChange.TokenLength : 0) -
351350
CurrentChange.StartOfTokenColumn;
351+
ScopeStack = {CurrentChange.indentAndNestingLevel()};
352352
CurrentChange.Spaces += Shift;
353353
}
354354

355355
if (Shift == 0)
356356
continue;
357357

358-
// This is for function parameters that are split across multiple lines,
359-
// as mentioned in the ScopeStack comment.
360-
if (InsideNestedScope && CurrentChange.NewlinesBefore > 0) {
361-
unsigned ScopeStart = ScopeStack.back();
362-
auto ShouldShiftBeAdded = [&] {
363-
// Function declaration
364-
if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName))
365-
return true;
366-
367-
// Lambda.
368-
if (Changes[ScopeStart - 1].Tok->is(TT_LambdaLBrace))
369-
return false;
370-
371-
// Continued function declaration
372-
if (ScopeStart > Start + 1 &&
373-
Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)) {
374-
return true;
375-
}
376-
377-
// Continued (template) function call.
378-
if (ScopeStart > Start + 1 &&
379-
Changes[ScopeStart - 2].Tok->isOneOf(tok::identifier,
380-
TT_TemplateCloser) &&
381-
Changes[ScopeStart - 1].Tok->is(tok::l_paren) &&
382-
Changes[ScopeStart].Tok->isNot(TT_LambdaLSquare)) {
383-
if (CurrentChange.Tok->MatchingParen &&
384-
CurrentChange.Tok->MatchingParen->is(TT_LambdaLBrace)) {
385-
return false;
386-
}
387-
if (Changes[ScopeStart].NewlinesBefore > 0)
388-
return false;
389-
if (CurrentChange.Tok->is(tok::l_brace) &&
390-
CurrentChange.Tok->is(BK_BracedInit)) {
391-
return true;
392-
}
393-
return Style.BinPackArguments;
394-
}
395-
396-
// Ternary operator
397-
if (CurrentChange.Tok->is(TT_ConditionalExpr))
398-
return true;
399-
400-
// Period Initializer .XXX = 1.
401-
if (CurrentChange.Tok->is(TT_DesignatedInitializerPeriod))
402-
return true;
403-
404-
// Continued ternary operator
405-
if (CurrentChange.Tok->Previous &&
406-
CurrentChange.Tok->Previous->is(TT_ConditionalExpr)) {
407-
return true;
408-
}
409-
410-
// Continued direct-list-initialization using braced list.
411-
if (ScopeStart > Start + 1 &&
412-
Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
413-
Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
414-
CurrentChange.Tok->is(tok::l_brace) &&
415-
CurrentChange.Tok->is(BK_BracedInit)) {
416-
return true;
417-
}
418-
419-
// Continued braced list.
420-
if (ScopeStart > Start + 1 &&
421-
Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&
422-
Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
423-
CurrentChange.Tok->isNot(tok::r_brace)) {
424-
for (unsigned OuterScopeStart : llvm::reverse(ScopeStack)) {
425-
// Lambda.
426-
if (OuterScopeStart > Start &&
427-
Changes[OuterScopeStart - 1].Tok->is(TT_LambdaLBrace)) {
428-
return false;
429-
}
430-
}
431-
if (Changes[ScopeStart].NewlinesBefore > 0)
432-
return false;
433-
return true;
434-
}
435-
436-
// Continued template parameter.
437-
if (Changes[ScopeStart - 1].Tok->is(TT_TemplateOpener))
438-
return true;
439-
440-
return false;
441-
};
442-
443-
if (ShouldShiftBeAdded())
444-
CurrentChange.Spaces += Shift;
445-
}
446-
447-
if (ContinuedStringLiteral)
358+
// This is for lines that are split across multiple lines, as mentioned in
359+
// the ScopeStack comment. The stack size being 1 means that the token is
360+
// not in a scope that should not move.
361+
if (ScopeStack.size() == 1u && CurrentChange.NewlinesBefore > 0 &&
362+
(ContinuedStringLiteral || InsideNestedScope)) {
448363
CurrentChange.Spaces += Shift;
364+
}
449365

450366
// We should not remove required spaces unless we break the line before.
451367
assert(Shift > 0 || Changes[i].NewlinesBefore > 0 ||

‎clang/unittests/Format/FormatTest.cpp‎

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19539,6 +19539,15 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
1953919539
"int j = 2;",
1954019540
Alignment);
1954119541

19542+
verifyFormat("int abcdefghijk = 111;\n"
19543+
"auto lambda = [] {\n"
19544+
" int c = call(1, //\n"
19545+
" 2, //\n"
19546+
" 3, //\n"
19547+
" 4);\n"
19548+
"};",
19549+
Alignment);
19550+
1954219551
verifyFormat("template <typename T, typename T_0 = very_long_type_name_0,\n"
1954319552
" typename B = very_long_type_name_1,\n"
1954419553
" typename T_2 = very_long_type_name_2>\n"
@@ -19575,6 +19584,12 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
1957519584
" return;\n"
1957619585
"};",
1957719586
Alignment);
19587+
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
19588+
"auto b = g([] {\n"
19589+
" return \"Hello \"\n"
19590+
" \"World\";\n"
19591+
"});",
19592+
Alignment);
1957819593
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
1957919594
"auto b = g([] {\n"
1958019595
" f();\n"
@@ -19599,12 +19614,11 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
1959919614
" ccc ? aaaaa : bbbbb,\n"
1960019615
" dddddddddddddddddddddddddd);",
1960119616
Alignment);
19602-
// FIXME: https://llvm.org/PR53497
19603-
// verifyFormat("auto aaaaaaaaaaaa = f();\n"
19604-
// "auto b = f(aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
19605-
// " ccc ? aaaaa : bbbbb,\n"
19606-
// " dddddddddddddddddddddddddd);",
19607-
// Alignment);
19617+
verifyFormat("auto aaaaaaaaaaaa = f();\n"
19618+
"auto b = f(aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
19619+
" ccc ? aaaaa : bbbbb,\n"
19620+
" dddddddddddddddddddddddddd);",
19621+
Alignment);
1960819622

1960919623
// Confirm proper handling of AlignConsecutiveAssignments with
1961019624
// BinPackArguments.
@@ -20192,6 +20206,11 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) {
2019220206
" i = 3 //\n"
2019320207
"};",
2019420208
Alignment);
20209+
// When assignments are nested, each level should be aligned.
20210+
verifyFormat("float i2 = 0;\n"
20211+
"auto v = type{i2 = 1, //\n"
20212+
" i = 3};",
20213+
Alignment);
2019520214
Alignment.AlignConsecutiveAssignments.Enabled = false;
2019620215

2019720216
verifyFormat(
@@ -20681,6 +20700,21 @@ TEST_F(FormatTest, AlignWithLineBreaks) {
2068120700
"}",
2068220701
Style);
2068320702

20703+
verifyFormat("void foo() {\n"
20704+
" int myVar = 5;\n"
20705+
" double x = 3.14;\n"
20706+
" auto str = (\"Hello \"\n"
20707+
" \"World\");\n"
20708+
" auto s = (\"Hello \"\n"
20709+
" \"Again\");\n"
20710+
"}",
20711+
Style);
20712+
20713+
verifyFormat("A B = {\"Hello \"\n"
20714+
" \"World\"};\n"
20715+
"BYTE payload = 2;",
20716+
Style);
20717+
2068420718
// clang-format off
2068520719
verifyFormat("void foo() {\n"
2068620720
" const int capacityBefore = Entries.capacity();\n"
@@ -20763,6 +20797,28 @@ TEST_F(FormatTest, AlignWithInitializerPeriods) {
2076320797
"}",
2076420798
Style);
2076520799

20800+
// The lines inside the braces are supposed to be indented by
20801+
// BracedInitializerIndentWidth from the start of the line. They should not
20802+
// move with the opening brace.
20803+
verifyFormat("void foo2(void) {\n"
20804+
" BYTE p[1] = 1;\n"
20805+
" A B = {\n"
20806+
" .one_foooooooooooooooo = 2,\n"
20807+
" .two_fooooooooooooo = 3,\n"
20808+
" .three_fooooooooooooo = 4,\n"
20809+
" };\n"
20810+
" BYTE payload = 2;\n"
20811+
"}",
20812+
Style);
20813+
20814+
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
20815+
"auto b = g([] {\n"
20816+
" x = {.one_foooooooooooooooo = 2, //\n"
20817+
" .two_fooooooooooooo = 3, //\n"
20818+
" .three_fooooooooooooo = 4};\n"
20819+
"});",
20820+
Style);
20821+
2076620822
Style.AlignConsecutiveAssignments.Enabled = false;
2076720823
Style.AlignConsecutiveDeclarations.Enabled = true;
2076820824
verifyFormat("void foo3(void) {\n"

0 commit comments

Comments
 (0)