Format closing braces when reformatting the line containing theopening brace.

[klimek]

Summary

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();

Format closing braces.

[krasimir]

Great patch!

lib/Format/AffectedRangeManager.cpp

144 ↗

(On Diff #142787)

nit: replace -1 with UnwrappedLine::kInvalidIndex

lib/Format/UnwrappedLineParser.h

58 ↗

(On Diff #142787)

This comment could be improved. I interpret it as: start with the line at MatchingOpeningBlockLineIndex, and store the closing block line index of it, which will result in just the index of the current line being stored here if it closes a block.

[krasimir]

Another point: for the example in the summary about bailing-out early, is there a test for this already? If not, we should add one.

[klimek]

Address comments.

[klimek]

In D45726#1071925, @krasimir wrote:

Another point: for the example in the summary about bailing-out early, is there a test for this already? If not, we should add one.

Yep, there already is one - I regressed that with my change at first ;)

cfe/trunk/lib/Format/AffectedRangeManager.h

 
 class AffectedRangeManager {
 public:
   AffectedRangeManager(const SourceManager &SourceMgr,
                        const ArrayRef<CharSourceRange> Ranges)
       : 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<AnnotatedLine *>::iterator I,
-                            SmallVectorImpl<AnnotatedLine *>::iterator E);
+  bool computeAffectedLines(SmallVectorImpl<AnnotatedLine *> &Lines);
 
   // Returns true if 'Range' intersects with one of the input ranges.
   bool affectsCharSourceRange(const CharSourceRange &Range);
 
 private:
   // Returns true if the range from 'First' to 'Last' intersects with one of the
   // input ranges.
   bool affectsTokenRange(const FormatToken &First, const FormatToken &Last,
                          bool IncludeLeadingNewlines);
 
   // Returns true if one of the input ranges intersect the leading empty lines
   // before 'Tok'.
   bool affectsLeadingEmptyLines(const FormatToken &Tok);
 
   // Marks all lines between I and E as well as all their children as affected.
   void markAllAsAffected(SmallVectorImpl<AnnotatedLine *>::iterator I,
                          SmallVectorImpl<AnnotatedLine *>::iterator E);
 
   // 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<AnnotatedLine *> &Lines);
 
   const SourceManager &SourceMgr;
   const SmallVector<CharSourceRange, 8> Ranges;
 };
 
 } // namespace format
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_FORMAT_AFFECTEDRANGEMANAGER_H

cfe/trunk/lib/Format/AffectedRangeManager.cpp

 
 #include "FormatToken.h"
 #include "TokenAnnotator.h"
 
 namespace clang {
 namespace format {
 
 bool AffectedRangeManager::computeAffectedLines(
-    SmallVectorImpl<AnnotatedLine *>::iterator I,
-    SmallVectorImpl<AnnotatedLine *>::iterator E) {
+    SmallVectorImpl<AnnotatedLine *> &Lines) {
+  SmallVectorImpl<AnnotatedLine *>::iterator I = Lines.begin();
+  SmallVectorImpl<AnnotatedLine *>::iterator E = Lines.end();
   bool SomeLineAffected = false;
   const AnnotatedLine *PreviousLine = nullptr;
   while (I != E) {
     AnnotatedLine *Line = *I;
     Line->LeadingEmptyLinesAffected = affectsLeadingEmptyLines(*Line->First);
 
     // If a line is part of a preprocessor directive, it needs to be formatted
     // if any token within the directive is affected.
if (Line->InPPDirective) {
                             /*IncludeLeadingNewlines=*/false)) {
         SomeLineAffected = true;
         markAllAsAffected(I, PPEnd);
       }
       I = PPEnd;
       continue;
     }
 
-    if (nonPPLineAffected(Line, PreviousLine))
+    if (nonPPLineAffected(Line, PreviousLine, Lines))
       SomeLineAffected = true;
 
     PreviousLine = Line;
     ++I;
   }
   return SomeLineAffected;
 }
 
void AffectedRangeManager::markAllAsAffected(
   while (I != E) {
     (*I)->Affected = true;
     markAllAsAffected((*I)->Children.begin(), (*I)->Children.end());
     ++I;
   }
 }
 
 bool AffectedRangeManager::nonPPLineAffected(
-    AnnotatedLine *Line, const AnnotatedLine *PreviousLine) {
+    AnnotatedLine *Line, const AnnotatedLine *PreviousLine,
+    SmallVectorImpl<AnnotatedLine *> &Lines) {
   bool SomeLineAffected = false;
-  Line->ChildrenAffected =
-      computeAffectedLines(Line->Children.begin(), Line->Children.end());
+  Line->ChildrenAffected = computeAffectedLines(Line->Children);
   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;
bool AffectedRangeManager::nonPPLineAffected(
   bool LineMoved = PreviousLine && PreviousLine->Affected &&
                    Line->First->NewlinesBefore == 0;
 
   bool IsContinuedComment =
       Line->First->is(tok::comment) && Line->First->Next == nullptr &&
       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;
   }
   return SomeLineAffected;
 }
 
 } // namespace format
 } // namespace clang

cfe/trunk/lib/Format/Format.cpp

 public:
   JavaScriptRequoter(const Environment &Env, const FormatStyle &Style)
       : TokenAnalyzer(Env, Style) {}
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override {
-    AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
-                                          AnnotatedLines.end());
+    AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
     tooling::Replacements Result;
     requoteJSStringLiteral(AnnotatedLines, Result);
     return {Result, 0};
   }
 
 private:
   // Replaces double/single-quoted string literal as appropriate, re-escaping
   // the contents in the process.
Formatter(const Environment &Env, const FormatStyle &Style,
       : TokenAnalyzer(Env, Style), Status(Status) {}
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           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]);
     }
     Annotator.setCommentLineLevels(AnnotatedLines);
 
     WhitespaceManager Whitespaces(
         Env.getSourceManager(), Style,
         inputUsesCRLF(Env.getSourceManager().getBufferData(Env.getFileID())));
analyze(TokenAnnotator &Annotator,
           FormatTokenLexer &Tokens) override {
     // FIXME: in the current implementation the granularity of affected range
     // is an annotated line. However, this is not sufficient. Furthermore,
     // redundant code introduced by replacements does not necessarily
     // intercept with ranges of replacements that result in the redundancy.
     // 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);
 
     for (auto &Line : AnnotatedLines) {
       if (Line->Affected) {
         cleanupRight(Line->First, tok::comma, tok::comma);
         cleanupRight(Line->First, TT_CtorInitializerColon, tok::comma);
         cleanupRight(Line->First, tok::l_paren, tok::comma);

cfe/trunk/lib/Format/NamespaceEndCommentsFixer.cpp

 NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
                                                      const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 std::pair<tooling::Replacements, unsigned> NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &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;
   unsigned int CompactedNamespacesCount = 0;
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     const AnnotatedLine *EndLine = AnnotatedLines[I];
     const FormatToken *NamespaceTok =
         getNamespaceToken(EndLine, AnnotatedLines);

cfe/trunk/lib/Format/SortJavaScriptImports.cpp

JavaScriptImportSorter(const Environment &Env, const FormatStyle &Style)
       : TokenAnalyzer(Env, Style),
         FileContents(Env.getSourceManager().getBufferData(Env.getFileID())) {}
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override {
     tooling::Replacements Result;
-    AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
-                                          AnnotatedLines.end());
+    AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
 
     const AdditionalKeywords &Keywords = Tokens.getKeywords();
     SmallVector<JsModuleReference, 16> References;
     AnnotatedLine *FirstNonImportLine;
     std::tie(References, FirstNonImportLine) =
         parseModuleReferences(Keywords, AnnotatedLines);
 
     if (References.empty())

cfe/trunk/lib/Format/TokenAnnotator.h

enum LineType {
   LT_VirtualFunctionDecl
 };
 
 class AnnotatedLine {
 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),
         LeadingEmptyLinesAffected(false), ChildrenAffected(false),
         FirstStartColumn(Line.FirstStartColumn) {
     assert(!Line.Tokens.empty());
 
     // Calculate Next and Previous for all tokens. Note that we must overwrite
public:
   FormatToken *First;
   FormatToken *Last;
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
   size_t MatchingOpeningBlockLineIndex;
+  size_t MatchingClosingBlockLineIndex;
   bool InPPDirective;
   bool MustBeDeclaration;
   bool MightBeFunctionDecl;
   bool IsMultiVariableDeclStmt;
 
   /// \c True if this line should be formatted, i.e. intersects directly or
   /// indirectly with one of the input ranges.
   bool Affected;

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

bool MergeShortFunctions =
         (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
          I[1]->First->is(tok::r_brace)) ||
         (Style.AllowShortFunctionsOnASingleLine & FormatStyle::SFS_InlineOnly &&
          TheLine->Level != 0);
 
     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
           IndentTracker.skipLine(*I[i + 1]);
 
           Limit -= I[i + 1]->Last->TotalLength;
         }
         return i;
for (const AnnotatedLine *Line =
        Line; Line = NextLine, FirstLine = false) {
     const AnnotatedLine &TheLine = *Line;
     unsigned Indent = IndentTracker.getIndent();
 
     // We continue formatting unchanged lines to adjust their indent, e.g. if a
     // 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;
     bool ShouldFormat = TheLine.Affected || FixIndentation;
     // We cannot format this line; if the reason is that the line had a
     // parsing error, remember that.
     if (ShouldFormat && TheLine.Type == LT_Invalid && Status) {
       Status->FormatComplete = false;

cfe/trunk/lib/Format/UnwrappedLineParser.h

struct UnwrappedLine {
   bool InPPDirective;
 
   bool MustBeDeclaration;
 
   /// \brief If this \c UnwrappedLine closes a block in a sequence of lines,
   /// \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;
 
   unsigned FirstStartColumn = 0;
 };
 
 class UnwrappedLineConsumer {
 public:

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->Level = InitialLevel;
 
   if (PPStartHash == PPEndHash) {
     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;
     }
   }
 }
 
 static bool isGoogScope(const UnwrappedLine &Line) {
   // FIXME: Closure-library specific stuff should not be hard-coded but be
   // configurable.

cfe/trunk/lib/Format/UsingDeclarationsSorter.cpp

 UsingDeclarationsSorter::UsingDeclarationsSorter(const Environment &Env,
                                                  const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 std::pair<tooling::Replacements, unsigned> UsingDeclarationsSorter::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
   const SourceManager &SourceMgr = Env.getSourceManager();
-  AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
-                                        AnnotatedLines.end());
+  AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
   tooling::Replacements Fixes;
   SmallVector<UsingDeclaration, 4> UsingDeclarations;
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     const auto *FirstTok = AnnotatedLines[I]->First;
     if (AnnotatedLines[I]->InPPDirective ||
         !AnnotatedLines[I]->startsWith(tok::kw_using) || FirstTok->Finalized) {
       endUsingDeclarationBlock(&UsingDeclarations, SourceMgr, &Fixes);
       continue;

cfe/trunk/unittests/Format/FormatTestSelective.cpp

EXPECT_EQ("int x;  // Format this line.\n"
             "int xx; //\n"
             "int xxxxx; //",
             format("int x; // Format this line.\n"
                    "int xx; //\n"
                    "int xxxxx; //",
                    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"
             "  int        j;\n"
             "});",
             format("DEBUG(   {\n"
                    "  int        i;\n"
                    "  int        j;\n"

 }
 
 TEST_F(FormatTestSelective, StopFormattingWhenLeavingScope) {
   EXPECT_EQ(
       "void f() {\n"
       "  if (a) {\n"
       "    g();\n"
       "    h();\n"
-      "}\n"
+      "  }\n"
       "\n"
       "void g() {\n"
       "}",
       format("void f() {\n"
              "  if (a) {\n" // Assume this was added without the closing brace.
              "  g();\n"
              "  h();\n"
              "}\n"