Skip to content

Commit e220825

Browse files
sstwcwsstwcw
committed
[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, }}; ```
1 parent 36dc2a9 commit e220825

File tree

2 files changed

+74
-102
lines changed

2 files changed

+74
-102
lines changed

‎clang/lib/Format/WhitespaceManager.cpp‎

Lines changed: 26 additions & 102 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,104 +348,29 @@ 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();
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) {
362362
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;
363+
if (ContinuedStringLiteral)
433364
return true;
434-
}
435-
436-
// Continued template parameter.
437-
if (Changes[ScopeStart - 1].Tok->is(TT_TemplateOpener))
365+
if (InsideNestedScope && CurrentChange.IsAligned)
438366
return true;
439-
440367
return false;
441368
};
442369

443370
if (ShouldShiftBeAdded())
444371
CurrentChange.Spaces += Shift;
445372
}
446373

447-
if (ContinuedStringLiteral)
448-
CurrentChange.Spaces += Shift;
449-
450374
// We should not remove required spaces unless we break the line before.
451375
assert(Shift > 0 || Changes[i].NewlinesBefore > 0 ||
452376
CurrentChange.Spaces >=

‎clang/unittests/Format/FormatTest.cpp‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19575,6 +19575,12 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
1957519575
" return;\n"
1957619576
"};",
1957719577
Alignment);
19578+
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
19579+
"auto b = g([] {\n"
19580+
" return \"Hello \"\n"
19581+
" \"World\";\n"
19582+
"});",
19583+
Alignment);
1957819584
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
1957919585
"auto b = g([] {\n"
1958019586
" f();\n"
@@ -20192,6 +20198,11 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) {
2019220198
" i = 3 //\n"
2019320199
"};",
2019420200
Alignment);
20201+
// When assignments are nested, each level should be aligned.
20202+
verifyFormat("float i2 = 0;\n"
20203+
"auto v = type{i2 = 1, //\n"
20204+
" i = 3};",
20205+
Alignment);
2019520206
Alignment.AlignConsecutiveAssignments.Enabled = false;
2019620207

2019720208
verifyFormat(
@@ -20681,6 +20692,21 @@ TEST_F(FormatTest, AlignWithLineBreaks) {
2068120692
"}",
2068220693
Style);
2068320694

20695+
verifyFormat("void foo() {\n"
20696+
" int myVar = 5;\n"
20697+
" double x = 3.14;\n"
20698+
" auto str = (\"Hello \"\n"
20699+
" \"World\");\n"
20700+
" auto s = (\"Hello \"\n"
20701+
" \"Again\");\n"
20702+
"}",
20703+
Style);
20704+
20705+
verifyFormat("A B = {\"Hello \"\n"
20706+
" \"World\"};\n"
20707+
"BYTE payload = 2;",
20708+
Style);
20709+
2068420710
// clang-format off
2068520711
verifyFormat("void foo() {\n"
2068620712
" const int capacityBefore = Entries.capacity();\n"
@@ -20763,6 +20789,28 @@ TEST_F(FormatTest, AlignWithInitializerPeriods) {
2076320789
"}",
2076420790
Style);
2076520791

20792+
// The lines inside the braces are supposed to be indented by
20793+
// BracedInitializerIndentWidth from the start of the line. They should not
20794+
// move with the opening brace.
20795+
verifyFormat("void foo2(void) {\n"
20796+
" BYTE p[1] = 1;\n"
20797+
" A B = {\n"
20798+
" .one_foooooooooooooooo = 2,\n"
20799+
" .two_fooooooooooooo = 3,\n"
20800+
" .three_fooooooooooooo = 4,\n"
20801+
" };\n"
20802+
" BYTE payload = 2;\n"
20803+
"}",
20804+
Style);
20805+
20806+
verifyFormat("auto aaaaaaaaaaaaaaaaaaaaa = {};\n"
20807+
"auto b = g([] {\n"
20808+
" x = {.one_foooooooooooooooo = 2, //\n"
20809+
" .two_fooooooooooooo = 3, //\n"
20810+
" .three_fooooooooooooo = 4};\n"
20811+
"});",
20812+
Style);
20813+
2076620814
Style.AlignConsecutiveAssignments.Enabled = false;
2076720815
Style.AlignConsecutiveDeclarations.Enabled = true;
2076820816
verifyFormat("void foo3(void) {\n"

0 commit comments

Comments
 (0)