[clang-format] Continue aligned lines better #161903
Open
+74
−110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,
}};
```
|
@llvm/pr-subscribers-clang-format Author: None (sstwcw) Changesafter with config 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 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 The old algorithm had problems like these.
The following problems are not fixed yet. A token that opens a scope is needed. Operator precedence is not enough. 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 auto aaaaaaaaaaaaaaaaaaaaa = {};
auto b = {.a = {
.a = 0,
}};Full diff: https://github.com/llvm/llvm-project/pull/161903.diff 2 Files Affected:
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<WhitespaceManager::Change, 16> &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<unsigned, 16> ScopeStack;
+ SmallVector<std::tuple<unsigned, unsigned, unsigned>, 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"
|
|
I took a look at the remaining problems mentioned in the commit message. It seems that fixing them may require using other properties instead of |
|
Do you have an idea? Otherwise I'd say an improvement is better than nothing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
after with config
{AlignConsecutiveAssignments: true}before
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
IsAlignedattribute. 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.
The algorithm has trouble when things that should not move and things that should move are nested. This is related to the
IsAlignedattribute being a boolean. It also affects how spaces and tabs are selected.