From 0dddcf78b89bd062a8de0cd41ce4903ee93b25c0 Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Mon, 23 Apr 2018 09:34:26 +0000 Subject: [PATCH] Format closing braces when reformatting the line containing the opening brace. This required a couple of yaks to be shaved: 1. MatchingOpeningBlockLineIndex was misused to also store the closing index; instead, use a second variable, as this doesn't work correctly for "} else {". 2. We needed to change the API of AffectedRangeManager to not use iterators; we always passed in begin / end for the whole container before, so there was no mismatch in generality. 3. We need an extra check to discontinue formatting at the top level, as we now sometimes change the indent of the closing brace, but want to bail out immediately afterwards, for example: void f() { if (a) { } void g(); Previously: void f() { if (a) { } void g(); Now: void f() { if (a) { } void g(); Differential Revision: https://reviews.llvm.org/D45726 llvm-svn: 330573 --- clang/lib/Format/AffectedRangeManager.cpp | 20 +++++--- clang/lib/Format/AffectedRangeManager.h | 9 ++-- clang/lib/Format/Format.cpp | 9 ++-- clang/lib/Format/NamespaceEndCommentsFixer.cpp | 3 +- clang/lib/Format/SortJavaScriptImports.cpp | 3 +- clang/lib/Format/TokenAnnotator.h | 2 + clang/lib/Format/UnwrappedLineFormatter.cpp | 9 ++-- clang/lib/Format/UnwrappedLineParser.cpp | 2 +- clang/lib/Format/UnwrappedLineParser.h | 6 ++- clang/lib/Format/UsingDeclarationsSorter.cpp | 3 +- clang/unittests/Format/FormatTestSelective.cpp | 68 +++++++++++++++++++++++++- 11 files changed, 104 insertions(+), 30 deletions(-) diff --git a/clang/lib/Format/AffectedRangeManager.cpp b/clang/lib/Format/AffectedRangeManager.cpp index 5d4df19..02e9f5e 100644 --- a/clang/lib/Format/AffectedRangeManager.cpp +++ b/clang/lib/Format/AffectedRangeManager.cpp @@ -21,8 +21,9 @@ namespace clang { namespace format { bool AffectedRangeManager::computeAffectedLines( - SmallVectorImpl::iterator I, - SmallVectorImpl::iterator E) { + SmallVectorImpl &Lines) { + SmallVectorImpl::iterator I = Lines.begin(); + SmallVectorImpl::iterator E = Lines.end(); bool SomeLineAffected = false; const AnnotatedLine *PreviousLine = nullptr; while (I != E) { @@ -48,7 +49,7 @@ bool AffectedRangeManager::computeAffectedLines( continue; } - if (nonPPLineAffected(Line, PreviousLine)) + if (nonPPLineAffected(Line, PreviousLine, Lines)) SomeLineAffected = true; PreviousLine = Line; @@ -99,10 +100,10 @@ void AffectedRangeManager::markAllAsAffected( } bool AffectedRangeManager::nonPPLineAffected( - AnnotatedLine *Line, const AnnotatedLine *PreviousLine) { + AnnotatedLine *Line, const AnnotatedLine *PreviousLine, + SmallVectorImpl &Lines) { bool SomeLineAffected = false; - Line->ChildrenAffected = - computeAffectedLines(Line->Children.begin(), Line->Children.end()); + Line->ChildrenAffected = computeAffectedLines(Line->Children); if (Line->ChildrenAffected) SomeLineAffected = true; @@ -138,8 +139,13 @@ bool AffectedRangeManager::nonPPLineAffected( Line->First->NewlinesBefore < 2 && PreviousLine && PreviousLine->Affected && PreviousLine->Last->is(tok::comment); + bool IsAffectedClosingBrace = + Line->First->is(tok::r_brace) && + Line->MatchingOpeningBlockLineIndex != UnwrappedLine::kInvalidIndex && + Lines[Line->MatchingOpeningBlockLineIndex]->Affected; + if (SomeTokenAffected || SomeFirstChildAffected || LineMoved || - IsContinuedComment) { + IsContinuedComment || IsAffectedClosingBrace) { Line->Affected = true; SomeLineAffected = true; } diff --git a/clang/lib/Format/AffectedRangeManager.h b/clang/lib/Format/AffectedRangeManager.h index d8d5ee5..b9a0cad 100644 --- a/clang/lib/Format/AffectedRangeManager.h +++ b/clang/lib/Format/AffectedRangeManager.h @@ -30,10 +30,9 @@ public: : SourceMgr(SourceMgr), Ranges(Ranges.begin(), Ranges.end()) {} // Determines which lines are affected by the SourceRanges given as input. - // Returns \c true if at least one line between I and E or one of their + // Returns \c true if at least one line in \p Lines or one of their // children is affected. - bool computeAffectedLines(SmallVectorImpl::iterator I, - SmallVectorImpl::iterator E); + bool computeAffectedLines(SmallVectorImpl &Lines); // Returns true if 'Range' intersects with one of the input ranges. bool affectsCharSourceRange(const CharSourceRange &Range); @@ -54,8 +53,8 @@ private: // 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, - const AnnotatedLine *PreviousLine); + bool nonPPLineAffected(AnnotatedLine *Line, const AnnotatedLine *PreviousLine, + SmallVectorImpl &Lines); const SourceManager &SourceMgr; const SmallVector Ranges; diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp index 98b2656..e2d2565 100644 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -1006,8 +1006,7 @@ public: analyze(TokenAnnotator &Annotator, SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); tooling::Replacements Result; requoteJSStringLiteral(AnnotatedLines, Result); return {Result, 0}; @@ -1097,8 +1096,7 @@ public: FormatTokenLexer &Tokens) override { tooling::Replacements Result; deriveLocalStyle(AnnotatedLines); - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) { Annotator.calculateFormattingInformation(*AnnotatedLines[i]); } @@ -1222,8 +1220,7 @@ public: // To determine if some redundant code is actually introduced by // replacements(e.g. deletions), we need to come up with a more // sophisticated way of computing affected ranges. - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); checkEmptyNamespace(AnnotatedLines); diff --git a/clang/lib/Format/NamespaceEndCommentsFixer.cpp b/clang/lib/Format/NamespaceEndCommentsFixer.cpp index f5832a4..6311c05 100644 --- a/clang/lib/Format/NamespaceEndCommentsFixer.cpp +++ b/clang/lib/Format/NamespaceEndCommentsFixer.cpp @@ -141,8 +141,7 @@ std::pair NamespaceEndCommentsFixer::analyze( TokenAnnotator &Annotator, SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) { const SourceManager &SourceMgr = Env.getSourceManager(); - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); tooling::Replacements Fixes; std::string AllNamespaceNames = ""; size_t StartLineIndex = SIZE_MAX; diff --git a/clang/lib/Format/SortJavaScriptImports.cpp b/clang/lib/Format/SortJavaScriptImports.cpp index d0b979e..b598a26 100644 --- a/clang/lib/Format/SortJavaScriptImports.cpp +++ b/clang/lib/Format/SortJavaScriptImports.cpp @@ -128,8 +128,7 @@ public: SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) override { tooling::Replacements Result; - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); const AdditionalKeywords &Keywords = Tokens.getKeywords(); SmallVector References; diff --git a/clang/lib/Format/TokenAnnotator.h b/clang/lib/Format/TokenAnnotator.h index 04a18d4..7be0753c 100644 --- a/clang/lib/Format/TokenAnnotator.h +++ b/clang/lib/Format/TokenAnnotator.h @@ -40,6 +40,7 @@ public: AnnotatedLine(const UnwrappedLine &Line) : First(Line.Tokens.front().Tok), Level(Line.Level), MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex), + MatchingClosingBlockLineIndex(Line.MatchingClosingBlockLineIndex), InPPDirective(Line.InPPDirective), MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false), IsMultiVariableDeclStmt(false), Affected(false), @@ -112,6 +113,7 @@ public: LineType Type; unsigned Level; size_t MatchingOpeningBlockLineIndex; + size_t MatchingClosingBlockLineIndex; bool InPPDirective; bool MustBeDeclaration; bool MightBeFunctionDecl; diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index 953a5d3..45ddc1c 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -252,9 +252,9 @@ private: if (Style.CompactNamespaces) { if (isNamespaceDeclaration(TheLine)) { int i = 0; - unsigned closingLine = TheLine->MatchingOpeningBlockLineIndex - 1; + unsigned closingLine = TheLine->MatchingClosingBlockLineIndex - 1; for (; I + 1 + i != E && isNamespaceDeclaration(I[i + 1]) && - closingLine == I[i + 1]->MatchingOpeningBlockLineIndex && + closingLine == I[i + 1]->MatchingClosingBlockLineIndex && I[i + 1]->Last->TotalLength < Limit; i++, closingLine--) { // No extra indent for compacted namespaces @@ -1033,9 +1033,12 @@ UnwrappedLineFormatter::format(const SmallVectorImpl &Lines, // scope was added. However, we need to carefully stop doing this when we // exit the scope of affected lines to prevent indenting a the entire // remaining file if it currently missing a closing brace. + bool PreviousRBrace = + PreviousLine && PreviousLine->startsWith(tok::r_brace); bool ContinueFormatting = TheLine.Level > RangeMinLevel || - (TheLine.Level == RangeMinLevel && !TheLine.startsWith(tok::r_brace)); + (TheLine.Level == RangeMinLevel && !PreviousRBrace && + !TheLine.startsWith(tok::r_brace)); bool FixIndentation = (FixBadIndentation || ContinueFormatting) && Indent != TheLine.First->OriginalColumn; diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp index b61ffb2..b5bc80b 100644 --- a/clang/lib/Format/UnwrappedLineParser.cpp +++ b/clang/lib/Format/UnwrappedLineParser.cpp @@ -570,7 +570,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel, Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) { // Update the opening line to add the forward reference as well - (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex = + (*CurrentLines)[OpeningLineIndex].MatchingClosingBlockLineIndex = CurrentLines->size() - 1; } } diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h index b876735..da7529c 100644 --- a/clang/lib/Format/UnwrappedLineParser.h +++ b/clang/lib/Format/UnwrappedLineParser.h @@ -53,7 +53,11 @@ struct UnwrappedLine { /// \c MatchingOpeningBlockLineIndex stores the index of the corresponding /// opening line. Otherwise, \c MatchingOpeningBlockLineIndex must be /// \c kInvalidIndex. - size_t MatchingOpeningBlockLineIndex; + size_t MatchingOpeningBlockLineIndex = kInvalidIndex; + + /// \brief If this \c UnwrappedLine opens a block, stores the index of the + /// line with the corresponding closing brace. + size_t MatchingClosingBlockLineIndex = kInvalidIndex; static const size_t kInvalidIndex = -1; diff --git a/clang/lib/Format/UsingDeclarationsSorter.cpp b/clang/lib/Format/UsingDeclarationsSorter.cpp index ef0c7a7..d7ab4f3 100644 --- a/clang/lib/Format/UsingDeclarationsSorter.cpp +++ b/clang/lib/Format/UsingDeclarationsSorter.cpp @@ -187,8 +187,7 @@ std::pair UsingDeclarationsSorter::analyze( TokenAnnotator &Annotator, SmallVectorImpl &AnnotatedLines, FormatTokenLexer &Tokens) { const SourceManager &SourceMgr = Env.getSourceManager(); - AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(), - AnnotatedLines.end()); + AffectedRangeMgr.computeAffectedLines(AnnotatedLines); tooling::Replacements Fixes; SmallVector UsingDeclarations; for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) { diff --git a/clang/unittests/Format/FormatTestSelective.cpp b/clang/unittests/Format/FormatTestSelective.cpp index 182218f..57d5daf 100644 --- a/clang/unittests/Format/FormatTestSelective.cpp +++ b/clang/unittests/Format/FormatTestSelective.cpp @@ -177,6 +177,72 @@ TEST_F(FormatTestSelective, FormatsCommentsLocally) { 0, 0)); } +TEST_F(FormatTestSelective, ContinueReindenting) { + // When we change an indent, we continue formatting as long as following + // lines are not indented correctly. + EXPECT_EQ("int i;\n" + "int b;\n" + "int c;\n" + "int d;\n" + "int e;\n" + " int f;\n", + format("int i;\n" + " int b;\n" + " int c;\n" + " int d;\n" + "int e;\n" + " int f;\n", + 11, 0)); +} + +TEST_F(FormatTestSelective, ReindentClosingBrace) { + EXPECT_EQ("int i;\n" + "int f() {\n" + " int a;\n" + " int b;\n" + "}\n" + " int c;\n", + format("int i;\n" + " int f(){\n" + "int a;\n" + "int b;\n" + " }\n" + " int c;\n", + 11, 0)); + EXPECT_EQ("void f() {\n" + " if (foo) {\n" + " b();\n" + " } else {\n" + " c();\n" + " }\n" + "int d;\n" + "}\n", + format("void f() {\n" + " if (foo) {\n" + "b();\n" + "}else{\n" + "c();\n" + "}\n" + "int d;\n" + "}\n", + 13, 0)); + EXPECT_EQ("int i = []() {\n" + " class C {\n" + " int a;\n" + " int b;\n" + " };\n" + " int c;\n" + "};\n", + format("int i = []() {\n" + " class C{\n" + "int a;\n" + "int b;\n" + "};\n" + "int c;\n" + " };\n", + 17, 0)); +} + TEST_F(FormatTestSelective, IndividualStatementsOfNestedBlocks) { EXPECT_EQ("DEBUG({\n" " int i;\n" @@ -503,7 +569,7 @@ TEST_F(FormatTestSelective, StopFormattingWhenLeavingScope) { " if (a) {\n" " g();\n" " h();\n" - "}\n" + " }\n" "\n" "void g() {\n" "}", -- 2.7.4