From 4f6f1f82e1f5709ae6d51d5c68b9cf0ef9855e16 Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Thu, 28 Nov 2013 15:58:55 +0000 Subject: [PATCH] clang-format: Improve selective formatting of nested statements. Previously, clang-format could create quite corrupt formattings if individual lines of nested blocks (e.g. in "DEBUG({})" or lambdas) were used. With this patch, it tries to extend the formatted regions to leave around some reasonable format without always formatting the entire surrounding statement. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@195925 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Format/Format.cpp | 83 ++++++++++++++++++++++++--------- lib/Format/TokenAnnotator.h | 5 +- unittests/Format/FormatTest.cpp | 58 +++++++++++++++++++---- 3 files changed, 112 insertions(+), 34 deletions(-) diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp index b5b3797f3a..a7d7d5eb10 100644 --- a/lib/Format/Format.cpp +++ b/lib/Format/Format.cpp @@ -560,7 +560,7 @@ public: Joiner(Style) {} unsigned format(const SmallVectorImpl &Lines, bool DryRun, - int AdditionalIndent = 0) { + int AdditionalIndent = 0, bool FixBadIndentation = false) { assert(!Lines.empty()); unsigned Penalty = 0; std::vector IndentForLevel; @@ -589,6 +589,9 @@ public: } I += MergedLines; + unsigned LevelIndent = getIndent(IndentForLevel, TheLine.Level); + bool FixIndentation = + FixBadIndentation && (LevelIndent != FirstTok->OriginalColumn); if (TheLine.First->is(tok::eof)) { if (PreviousLine && PreviousLine->Affected && !DryRun) { // Remove the file's trailing whitespace. @@ -597,8 +600,8 @@ public: /*IndentLevel=*/0, /*Spaces=*/0, /*TargetColumn=*/0); } - } else if (TheLine.Type != LT_Invalid && TheLine.Affected) { - unsigned LevelIndent = getIndent(IndentForLevel, TheLine.Level); + } else if (TheLine.Type != LT_Invalid && + (TheLine.Affected || FixIndentation)) { if (FirstTok->WhitespaceRange.isValid()) { if (!DryRun) formatFirstToken(*TheLine.First, PreviousLine, TheLine.Level, @@ -620,6 +623,7 @@ public: while (State.NextToken != NULL) Indenter->addTokenToState(State, /*Newline=*/false, DryRun); } else if (Style.ColumnLimit == 0) { + // FIXME: Implement nested blocks for ColumnLimit = 0. NoColumnLimitFormatter Formatter(Indenter); if (!DryRun) Formatter.format(Indent, &TheLine); @@ -628,6 +632,8 @@ public: } IndentForLevel[TheLine.Level] = LevelIndent; + } else if (TheLine.ChildrenAffected) { + format(TheLine.Children, DryRun); } else { // Format the first token if necessary, and notify the WhitespaceManager // about the unchanged whitespace. @@ -636,7 +642,7 @@ public: (Tok->NewlinesBefore > 0 || Tok->IsFirst)) { unsigned LevelIndent = Tok->OriginalColumn; if (!DryRun) { - // Remove trailing whitespace of the previous line if. + // Remove trailing whitespace of the previous line. if ((PreviousLine && PreviousLine->Affected) || TheLine.LeadingEmptyLinesAffected) { formatFirstToken(*Tok, PreviousLine, TheLine.Level, LevelIndent, @@ -924,7 +930,8 @@ private: if (NewLine) { int AdditionalIndent = State.Stack.back().Indent - Previous.Children[0]->Level * Style.IndentWidth; - Penalty += format(Previous.Children, DryRun, AdditionalIndent); + Penalty += format(Previous.Children, DryRun, AdditionalIndent, + /*FixBadIndentation=*/true); return true; } @@ -1263,7 +1270,8 @@ public: private: // Determines which lines are affected by the SourceRanges given as input. - // Returns \c true if at least one line between I and E was affected. + // Returns \c true if at least one line between I and E or one of their + // children is affected. bool computeAffectedLines(SmallVectorImpl::iterator I, SmallVectorImpl::iterator E) { bool SomeLineAffected = false; @@ -1291,30 +1299,59 @@ private: continue; } - bool SomeTokenAffected = false; - for (FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) { - bool IncludeLeadingNewlines = Tok != Line->First; - if (affectsTokenRange(*Tok, *Tok, IncludeLeadingNewlines)) { - SomeTokenAffected = true; - break; - } - } - bool SomeChildAffected = - computeAffectedLines(Line->Children.begin(), Line->Children.end()); - bool LineMoved = PreviousLineAffected && Line->First->NewlinesBefore == 0; - if (SomeTokenAffected || SomeChildAffected || LineMoved) { - Line->Affected = true; + if (nonPPLineAffected(Line, &PreviousLineAffected)) SomeLineAffected = true; - PreviousLineAffected = true; - } else { - PreviousLineAffected = false; - } ++I; } return SomeLineAffected; } + // Determines whether 'Line' is affected by the SourceRanges given as input. + // Returns \c true if line or one if its children is affected. + bool nonPPLineAffected(AnnotatedLine *Line, bool *PreviousLineAffected) { + bool SomeLineAffected = false; + Line->ChildrenAffected = + computeAffectedLines(Line->Children.begin(), Line->Children.end()); + if (Line->ChildrenAffected) + SomeLineAffected = true; + + // Stores whether one of the line's tokens is directly affected. + bool SomeTokenAffected = false; + // Stores whether we need to look at the leading newlines of the next token + // in order to determine whether it was affected. + bool IncludeLeadingNewlines = false; + + // Stores whether the first child line of any of this line's tokens is + // affected. + bool SomeFirstChildAffected = false; + + for (FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) { + // Determine whether 'Tok' was affected. + if (affectsTokenRange(*Tok, *Tok, IncludeLeadingNewlines)) + SomeTokenAffected = true; + + // Determine whether the first child of 'Tok' was affected. + if (!Tok->Children.empty() && Tok->Children.front()->Affected) + SomeFirstChildAffected = true; + + IncludeLeadingNewlines = Tok->Children.empty(); + } + + // Was this line moved, i.e. has it previously been on the same line as an + // affected line? + bool LineMoved = *PreviousLineAffected && Line->First->NewlinesBefore == 0; + + if (SomeTokenAffected || SomeFirstChildAffected || LineMoved) { + Line->Affected = true; + *PreviousLineAffected = true; + SomeLineAffected = true; + } else { + *PreviousLineAffected = false; + } + return SomeLineAffected; + } + // Marks all lines between I and E as well as all their children as affected. void markAllAsAffected(SmallVectorImpl::iterator I, SmallVectorImpl::iterator E) { diff --git a/lib/Format/TokenAnnotator.h b/lib/Format/TokenAnnotator.h index 8bdf758bc1..bd91980584 100644 --- a/lib/Format/TokenAnnotator.h +++ b/lib/Format/TokenAnnotator.h @@ -42,7 +42,7 @@ public: InPPDirective(Line.InPPDirective), MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false), StartsDefinition(false), Affected(false), - LeadingEmptyLinesAffected(false) { + LeadingEmptyLinesAffected(false), ChildrenAffected(false) { assert(!Line.Tokens.empty()); // Calculate Next and Previous for all tokens. Note that we must overwrite @@ -96,6 +96,9 @@ public: /// input ranges. bool LeadingEmptyLinesAffected; + /// \c True if a one of this line's children intersects with an input range. + bool ChildrenAffected; + private: // Disallow copying. AnnotatedLine(const AnnotatedLine &) LLVM_DELETED_FUNCTION; diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp index 350f8f2783..d809421a41 100644 --- a/unittests/Format/FormatTest.cpp +++ b/unittests/Format/FormatTest.cpp @@ -2511,6 +2511,36 @@ TEST_F(FormatTest, LayoutNestedBlocks) { " return;\n" " },\n" " a);", Style); +} + +TEST_F(FormatTest, IndividualStatementsOfNestedBlocks) { + EXPECT_EQ("DEBUG({\n" + " int i;\n" + " int j;\n" + "});", + format("DEBUG( {\n" + " int i;\n" + " int j;\n" + "} ) ;", + 20, 1, getLLVMStyle())); + EXPECT_EQ("DEBUG( {\n" + " int i;\n" + " int j;\n" + "} ) ;", + format("DEBUG( {\n" + " int i;\n" + " int j;\n" + "} ) ;", + 41, 1, getLLVMStyle())); + EXPECT_EQ("DEBUG({\n" + " int i;\n" + " int j;\n" + "});", + format("DEBUG( {\n" + " int i;\n" + " int j;\n" + "} ) ;", + 20, 1, getLLVMStyle())); EXPECT_EQ("Debug({\n" " if (aaaaaaaaaaaaaaaaaaaaaaaa)\n" @@ -2523,18 +2553,26 @@ TEST_F(FormatTest, LayoutNestedBlocks) { " },\n" " a);", 50, 1, getLLVMStyle())); -} - -TEST_F(FormatTest, IndividualStatementsOfNestedBlocks) { EXPECT_EQ("DEBUG({\n" - " int i;\n" - " int j;\n" + " DEBUG({\n" + " int a;\n" + " int b;\n" + " }) ;\n" "});", - format("DEBUG( {\n" - " int i;\n" - " int j;\n" - "} ) ;", - 40, 1, getLLVMStyle())); + format("DEBUG({\n" + " DEBUG({\n" + " int a;\n" + " int b;\n" // Format this line only. + " }) ;\n" // Don't touch this line. + "});", + 35, 0, getLLVMStyle())); + EXPECT_EQ("DEBUG({\n" + " int a; //\n" + "});", + format("DEBUG({\n" + " int a; //\n" + "});", + 0, 0, getLLVMStyle())); } TEST_F(FormatTest, PutEmptyBlocksIntoOneLine) { -- 2.40.0