Skip to content

Commit c43afcf

Browse files
committed
[clang-format] Align within the level with Cpp11BracedListStyle disabled
When the style is `{AlignConsecutiveDeclarations: true, Cpp11BracedListStyle: false}`, the program would sometimes align the lambda body with the outside. Like this. ```C++ const volatile auto result{ []() { const auto something = 1; return 2; } }; ``` This patch stops it. Now the output looks like this. ```C++ const volatile auto result{ []() { const auto something = 1; return 2; } }; ``` Fixes #53699. The problem was with how the `AlignTokenSequence` function tracked levels. The stack was pushed once when a token was more nested than the previous one. It was popped once when a token was less nested than the previous one. With the option `Cpp11BracedListStyle` disabled, the `[` token was deeper than the previous token by 2 levels. Both its indentation level and nesting level were more than that of the previous one. But the stack was only pushed once. The following tokens popped the 2 levels separately. The function treated the body of the lambda expression as on the same level as the outside. The problem is fixed this way. The function `AlignTokens` which calls the function `AlignTokenSequence` already uses a simpler and more robust way to track levels. It remembers the outermost level it should align. It compares the token's level with the outermost level. It does not need a stack. So it is immune to the problem. This patch makes the function `AlignTokenSequence` take as a parameter the indices of the tokens matched by the function `AlignTokens`. This way it does not have to contain the logic again. Now the function `AlignTokenSequence` only aligns tokens already matched by the function `AlignTokens`. It is easy to see that the assertion on line 453 holds. The workaround on line 354 is not needed any more. The test on line 20310 added at the same time as the assertion ensures that the assertion hold. The stack in the function `AlignTokenSequence` is kept for now. It is still used to figure out when things inside a level should move along with the outer level. Since the stack has the problem, the developer plans to move the logic into token annotator. It already uses a stack.
1 parent d0c0986 commit c43afcf

File tree

3 files changed

+34
-25
lines changed

3 files changed

+34
-25
lines changed

clang/lib/Format/TokenAnnotator.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6486,13 +6486,14 @@ void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) const {
64866486
<< "):\n";
64876487
const FormatToken *Tok = Line.First;
64886488
while (Tok) {
6489-
llvm::errs() << " M=" << Tok->MustBreakBefore
6489+
llvm::errs() << " I=" << Tok->IndentLevel << " M=" << Tok->MustBreakBefore
64906490
<< " C=" << Tok->CanBreakBefore
64916491
<< " T=" << getTokenTypeName(Tok->getType())
64926492
<< " S=" << Tok->SpacesRequiredBefore
64936493
<< " F=" << Tok->Finalized << " B=" << Tok->BlockParameterCount
64946494
<< " BK=" << Tok->getBlockKind() << " P=" << Tok->SplitPenalty
6495-
<< " Name=" << Tok->Tok.getName() << " L=" << Tok->TotalLength
6495+
<< " Name=" << Tok->Tok.getName() << " N=" << Tok->NestingLevel
6496+
<< " L=" << Tok->TotalLength
64966497
<< " PPK=" << Tok->getPackingKind() << " FakeLParens=";
64976498
for (prec::Level LParen : Tok->FakeLParens)
64986499
llvm::errs() << LParen << "/";

clang/lib/Format/WhitespaceManager.cpp

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
//===----------------------------------------------------------------------===//
1313

1414
#include "WhitespaceManager.h"
15+
#include "llvm/ADT/ArrayRef.h"
1516
#include "llvm/ADT/STLExtras.h"
1617
#include "llvm/ADT/SmallVector.h"
1718
#include <algorithm>
@@ -279,20 +280,19 @@ void WhitespaceManager::calculateLineBreakInformation() {
279280
}
280281

281282
// Align a single sequence of tokens, see AlignTokens below.
282-
// Column - The token for which Matches returns true is moved to this column.
283+
// Column - The tokens indexed in Matches are moved to this column.
283284
// RightJustify - Whether it is the token's right end or left end that gets
284285
// moved to that column.
285-
template <typename F>
286286
static void
287287
AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
288-
unsigned Column, bool RightJustify, F &&Matches,
288+
unsigned Column, bool RightJustify,
289+
ArrayRef<const unsigned> Matches,
289290
SmallVector<WhitespaceManager::Change, 16> &Changes) {
290-
bool FoundMatchOnLine = false;
291291
int Shift = 0;
292292

293293
// ScopeStack keeps track of the current scope depth. It contains indices of
294294
// the first token on each scope.
295-
// We only run the "Matches" function on tokens from the outer-most scope.
295+
// The "Matches" indices should only have tokens from the outer-most scope.
296296
// However, we do need to pay special attention to one class of tokens
297297
// that are not in the outer-most scope, and that is function parameters
298298
// which are split across multiple lines, as illustrated by this example:
@@ -314,6 +314,9 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
314314

315315
for (unsigned i = Start; i != End; ++i) {
316316
auto &CurrentChange = Changes[i];
317+
if (!Matches.empty() && Matches[0] < i)
318+
Matches.consume_front();
319+
assert(Matches.empty() || Matches[0] >= i);
317320
if (!ScopeStack.empty() &&
318321
CurrentChange.indentAndNestingLevel() <
319322
Changes[ScopeStack.back()].indentAndNestingLevel()) {
@@ -340,24 +343,15 @@ AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
340343

341344
if (CurrentChange.NewlinesBefore > 0 && !SkipMatchCheck) {
342345
Shift = 0;
343-
FoundMatchOnLine = false;
344346
}
345347

346348
// If this is the first matching token to be aligned, remember by how many
347349
// spaces it has to be shifted, so the rest of the changes on the line are
348350
// shifted by the same amount
349-
if (!FoundMatchOnLine && !SkipMatchCheck && Matches(CurrentChange)) {
350-
FoundMatchOnLine = true;
351+
if (!Matches.empty() && Matches[0] == i) {
351352
Shift = Column - (RightJustify ? CurrentChange.TokenLength : 0) -
352353
CurrentChange.StartOfTokenColumn;
353354
CurrentChange.Spaces += Shift;
354-
// FIXME: This is a workaround that should be removed when we fix
355-
// http://llvm.org/PR53699. An assertion later below verifies this.
356-
if (CurrentChange.NewlinesBefore == 0) {
357-
CurrentChange.Spaces =
358-
std::max(CurrentChange.Spaces,
359-
static_cast<int>(CurrentChange.Tok->SpacesRequiredBefore));
360-
}
361355
}
362356

363357
if (Shift == 0)
@@ -532,12 +526,14 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
532526
bool RightJustify = false) {
533527
// We arrange each line in 3 parts. The operator to be aligned (the anchor),
534528
// and text to its left and right. In the aligned text the width of each part
535-
// will be the maximum of that over the block that has been aligned. Maximum
536-
// widths of each part so far. When RightJustify is true and ACS.PadOperators
537-
// is false, the part from start of line to the right end of the anchor.
538-
// Otherwise, only the part to the left of the anchor. Including the space
539-
// that exists on its left from the start. Not including the padding added on
540-
// the left to right-justify the anchor.
529+
// will be the maximum of that over the block that has been aligned.
530+
531+
// Maximum widths of each part so far.
532+
// When RightJustify is true and ACS.PadOperators is false, the part from
533+
// start of line to the right end of the anchor. Otherwise, only the part to
534+
// the left of the anchor. Including the space that exists on its left from
535+
// the start. Not including the padding added on the left to right-justify the
536+
// anchor.
541537
unsigned WidthLeft = 0;
542538
// The operator to be aligned when RightJustify is true and ACS.PadOperators
543539
// is false. 0 otherwise.
@@ -550,6 +546,9 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
550546
unsigned StartOfSequence = 0;
551547
unsigned EndOfSequence = 0;
552548

549+
// The positions of the tokens to be aligned.
550+
SmallVector<unsigned> MatchedIndices;
551+
553552
// Measure the scope level (i.e. depth of (), [], {}) of the first token, and
554553
// abort when we hit any token in a higher scope than the starting one.
555554
auto IndentAndNestingLevel = StartAt < Changes.size()
@@ -578,14 +577,15 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
578577
auto AlignCurrentSequence = [&] {
579578
if (StartOfSequence > 0 && StartOfSequence < EndOfSequence) {
580579
AlignTokenSequence(Style, StartOfSequence, EndOfSequence,
581-
WidthLeft + WidthAnchor, RightJustify, Matches,
580+
WidthLeft + WidthAnchor, RightJustify, MatchedIndices,
582581
Changes);
583582
}
584583
WidthLeft = 0;
585584
WidthAnchor = 0;
586585
WidthRight = 0;
587586
StartOfSequence = 0;
588587
EndOfSequence = 0;
588+
MatchedIndices.clear();
589589
};
590590

591591
unsigned i = StartAt;
@@ -637,8 +637,10 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
637637

638638
// If there is more than one matching token per line, or if the number of
639639
// preceding commas, do not match anymore, end the sequence.
640-
if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch)
640+
if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch) {
641+
MatchedIndices.push_back(i);
641642
AlignCurrentSequence();
643+
}
642644

643645
CommasBeforeLastMatch = CommasBeforeMatch;
644646
FoundMatchOnLine = true;
@@ -684,6 +686,7 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
684686
WidthAnchor = NewAnchor;
685687
WidthRight = NewRight;
686688
}
689+
MatchedIndices.push_back(i);
687690
}
688691

689692
EndOfSequence = i;

clang/unittests/Format/FormatTest.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20312,6 +20312,11 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) {
2031220312
" return 2;\n"
2031320313
"} };",
2031420314
BracedAlign);
20315+
verifyFormat("const volatile auto result{ []() {\n"
20316+
" const auto something = 1;\n"
20317+
" return 2;\n"
20318+
"} };",
20319+
BracedAlign);
2031520320
verifyFormat("int foo{ []() {\n"
2031620321
" int bar{ 0 };\n"
2031720322
" return 0;\n"

0 commit comments

Comments
 (0)