[clang-format] Alternate merge of D21279

[daphnediane]

Summary

Alternate version of D21279 merged on changes from rL293616.

[daphnediane]

lib/Format/WhitespaceManager.cpp

88

Original version of the patch called propagateIndentLevels after the above line.

203

The original version of the patch had a TokenTypeAndLevel structure but the information needed was available just from the index, so that's all this version saves.

212

I tried just using NestingLevel here and below instead of the combined NestingLevel and IndentLevel and it doesn't work.

240

The original version of this code saved the TokenType in the stack and if made the decision on which token type to save based on if 2 back was a FunctionDeclarationName. This version moves that check to here where the token type is actually used.

418

Original version of patch used IsStartOfDeclName, equivalent logic is now done here.

lib/Format/WhitespaceManager.h

165

Is this still true? Moved this here from the propagateIndentLevels code in the original patch.

178

Original version of patch had a propagateIndentLevels here that is no longer need.

[daphnediane]

Changes have been combined into D21279

lib/Format/WhitespaceManager.h

struct Change {
     //
     // If this Change represents a continuation of a block comment,
     // \c StartOfBlockComment is pointer to the first Change in the block
     // comment. \c IndentationOffset is a relative column offset to this
     // change, so that the correct column can be reconstructed at the end of
     // the alignment process.
     const Change *StartOfBlockComment;
     int IndentationOffset;
+
+    std::pair<unsigned, unsigned> nestingAndIndentLevel() const {
+      // NestingLevel is raised on the opening paren/square, and
+      // remains raised until AFTER the closing paren/square. This
+      // means that with a statement like:
+      //
+      // for (int i = 0; ...)
+      //   int x = 1;.
+      //
+      // The "int x = 1" line is going to have the same NestingLevel
+      // as the tokens inside the parentheses of the "for" statement.
+      // In order to break this continuity, we force the closing
+      // paren/square to reduce it's nesting level.  If we don't do
+      // this, then "x = 1" ends up getting aligned to "i = 1"
+      return std::make_pair(
+          Tok->NestingLevel -
+              (Tok->isOneOf(tok::r_paren, tok::r_square) ? 1 : 0),
+          Tok->IndentLevel);
+    }
   };
 
 private:
   /// \brief Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
   /// \brief Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
   /// \brief Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
   /// \brief Align trailing comments over all \c Changes.
   void alignTrailingComments();

lib/Format/WhitespaceManager.cpp

Changes.push_back(
              std::max(0, Spaces), Newlines, PreviousPostfix, CurrentPrefix,
              InPPDirective && !Tok.IsFirst, /*IsInsideToken=*/true));
 }
 
 const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;

 
 // Align a single sequence of tokens, see AlignTokens below.
 template <typename F>
 static void
 AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
                    SmallVector<WhitespaceManager::Change, 16> &Changes) {
   bool FoundMatchOnLine = false;
   int Shift = 0;
+
+  // ScopeStack keeps track of the current scope depth.
+  // We only run the "Matches" function on tokens from the outer-most scope.
+  // However, we do need to adjust some special tokens within the entire
+  // Start..End range, regardless of their scope. This special rule applies
+  // only to function parameters which are split across multiple lines, and
+  // who's function names have been shifted for declaration alignment's sake.
+  // See "split function parameter alignment" in FormatTest.cpp for an example.
+  SmallVector<unsigned, 16> ScopeStack;
+
   for (unsigned i = Start; i != End; ++i) {
-    if (Changes[i].NewlinesBefore > 0) {
-      FoundMatchOnLine = false;
+    if (ScopeStack.size() != 0 &&
+        Changes[i].nestingAndIndentLevel() <
+            Changes[ScopeStack.back()].nestingAndIndentLevel())
+      ScopeStack.pop_back();
+
+    if (i != Start) {
+      if (Changes[i].nestingAndIndentLevel() >
+          Changes[i - 1].nestingAndIndentLevel()) {
+        ScopeStack.push_back(i);
+      }
+    }
+
+    bool InsideNestedScope = ScopeStack.size() != 0;
+
+    if (Changes[i].NewlinesBefore > 0 && !InsideNestedScope) {
       Shift = 0;
+      FoundMatchOnLine = false;
     }
 
     // If this is the first matching token to be aligned, remember by how many
     // spaces it has to be shifted, so the rest of the changes on the line are
     // shifted by the same amount
-    if (!FoundMatchOnLine && Matches(Changes[i])) {
+    if (!FoundMatchOnLine && !InsideNestedScope && Matches(Changes[i])) {
       FoundMatchOnLine = true;
       Shift = Column - Changes[i].StartOfTokenColumn;
       Changes[i].Spaces += Shift;
     }
 
+    // This is for function parameters that are split across multiple lines,
+    // as mentioned in the ScopeStack comment.
+    if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
+      unsigned ScopeStart = ScopeStack.back();
+      if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) ||
+          (ScopeStart > Start + 1 &&
+           Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)))
+        Changes[i].Spaces += Shift;
+    }
+
     assert(Shift >= 0);
     Changes[i].StartOfTokenColumn += Shift;
     if (i + 1 != Changes.size())
       Changes[i + 1].PreviousEndOfTokenColumn += Shift;
   }
 }
 
-// Walk through all of the changes and find sequences of matching tokens to
-// align. To do so, keep track of the lines and whether or not a matching token
-// was found on a line. If a matching token is found, extend the current
-// sequence. If the current line cannot be part of a sequence, e.g. because
-// there is an empty line before it or it contains only non-matching tokens,
-// finalize the previous sequence.
+// Walk through a subset of the changes, starting at StartAt, and find
+// sequences of matching tokens to align. To do so, keep track of the lines and
+// whether or not a matching token was found on a line. If a matching token is
+// found, extend the current sequence. If the current line cannot be part of a
+// sequence, e.g. because there is an empty line before it or it contains only
+// non-matching tokens, finalize the previous sequence.
+// The value returned is the token on which we stopped, either because we
+// exhausted all items inside Changes, or because we hit a scope level higher
+// than our initial scope.
+// This function is recursive. Each invocation processes only the scope level
+// equal to the initial level, which is the level of Changes[StartAt].
+// If we encounter a scope level greater than the initial level, then we call
+// ourselves recursively, thereby avoiding the pollution of the current state
+// with the alignment requirements of the nested sub-level. This recursive
+// behavior is necessary for aligning function prototypes that have one or more
+// arguments.
+// If this function encounters a scope level less than the initial level,
+// it exits.
+// There is a non-obvious subtlety in the recursive behavior: Even though we
+// defer processing of nested levels to recursive invocations of this
+// function, when it comes time to align a sequence of tokens, we run the
+// alignment on the entire sequence, including the nested levels.
+// When doing so, most of the nested tokens are skipped, because their
+// alignment was already handled by the recursive invocations of this function.
+// However, the special exception is that we do NOT skip function parameters
+// that are split across multiple lines. See the test case in FormatTest.cpp
+// that mentions "split function parameter alignment" for an example of this.
 template <typename F>
-static void AlignTokens(const FormatStyle &Style, F &&Matches,
-                        SmallVector<WhitespaceManager::Change, 16> &Changes) {
+static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
+                            SmallVector<WhitespaceManager::Change, 16> &Changes,
+                            unsigned StartAt) {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
 
   // Line number of the start and the end of the current token sequence.
   unsigned StartOfSequence = 0;
   unsigned EndOfSequence = 0;
 
-  // Keep track of the nesting level of matching tokens, i.e. the number of
-  // surrounding (), [], or {}. We will only align a sequence of matching
-  // token that share the same scope depth.
-  //
-  // FIXME: This could use FormatToken::NestingLevel information, but there is
-  // an outstanding issue wrt the brace scopes.
-  unsigned NestingLevelOfLastMatch = 0;
-  unsigned NestingLevel = 0;
+  // Measure the scope level (i.e. depth of (), [], {}) of the first token, and
+  // abort when we hit any token in a higher scope than the starting one.
+  auto NestingAndIndentLevel = StartAt < Changes.size()
+                                   ? Changes[StartAt].nestingAndIndentLevel()
+                                   : std::pair<unsigned, unsigned>(0, 0);
 
   // Keep track of the number of commas before the matching tokens, we will only
   // align a sequence of matching tokens if they are preceded by the same number
   // of commas.
   unsigned CommasBeforeLastMatch = 0;
   unsigned CommasBeforeMatch = 0;
 
   // Whether a matching token has been found on the current line.
if (StartOfSequence > 0 && StartOfSequence < EndOfSequence)
       AlignTokenSequence(StartOfSequence, EndOfSequence, MinColumn, Matches,
                          Changes);
     MinColumn = 0;
     MaxColumn = UINT_MAX;
     StartOfSequence = 0;
     EndOfSequence = 0;
   };
 
-  for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
+  unsigned i = StartAt;
+  for (unsigned e = Changes.size();
+       i != e && Changes[i].nestingAndIndentLevel() >= NestingAndIndentLevel;
+       ++i) {
+
     if (Changes[i].NewlinesBefore != 0) {
       CommasBeforeMatch = 0;
       EndOfSequence = i;
       // If there is a blank line, or if the last line didn't contain any
       // matching token, the sequence ends here.
       if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
         AlignCurrentSequence();
 
       FoundMatchOnLine = false;
     }
 
     if (Changes[i].Tok->is(tok::comma)) {
       ++CommasBeforeMatch;
-    } else if (Changes[i].Tok->isOneOf(tok::r_brace, tok::r_paren,
-                                       tok::r_square)) {
-      --NestingLevel;
-    } else if (Changes[i].Tok->isOneOf(tok::l_brace, tok::l_paren,
-                                       tok::l_square)) {
-      // We want sequences to skip over child scopes if possible, but not the
-      // other way around.
-      NestingLevelOfLastMatch = std::min(NestingLevelOfLastMatch, NestingLevel);
-      ++NestingLevel;
+    } else if (Changes[i].nestingAndIndentLevel() != NestingAndIndentLevel) {
+      // Call AlignTokens recursively, skipping over this scope block.
+      unsigned StoppedAt = AlignTokens(Style, Matches, Changes, i);
+      i = StoppedAt - 1;
+      continue;
     }
 
     if (!Matches(Changes[i]))
       continue;
 
     // If there is more than one matching token per line, or if the number of
     // preceding commas, or the scope depth, do not match anymore, end the
     // sequence.
-    if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch ||
-        NestingLevel != NestingLevelOfLastMatch)
+    if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch)
       AlignCurrentSequence();
 
     CommasBeforeLastMatch = CommasBeforeMatch;
-    NestingLevelOfLastMatch = NestingLevel;
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
       StartOfSequence = i;
 
     unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
     int LineLengthAfter = -Changes[i].Spaces;
     for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
       LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
     // If we are restricted by the maximum column width, end the sequence.
     if (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn ||
         CommasBeforeLastMatch != CommasBeforeMatch) {
       AlignCurrentSequence();
       StartOfSequence = i;
     }
 
     MinColumn = std::max(MinColumn, ChangeMinColumn);
     MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
   }
 
-  EndOfSequence = Changes.size();
+  unsigned StoppedAt = i;
+  EndOfSequence = i;
   AlignCurrentSequence();
+  return StoppedAt;
 }
 
 void WhitespaceManager::alignConsecutiveAssignments() {
   if (!Style.AlignConsecutiveAssignments)
     return;
 
   AlignTokens(Style,
               [&](const Change &C) {
                 // Do not align on equal signs that are first on a line.
                 if (C.NewlinesBefore > 0)
                   return false;
 
                 // Do not align on equal signs that are last on a line.
                 if (&C != &Changes.back() && (&C + 1)->NewlinesBefore > 0)
                   return false;
 
                 return C.Tok->is(tok::equal);
               },
-              Changes);
+              Changes, 0);
 }
 
 void WhitespaceManager::alignConsecutiveDeclarations() {
   if (!Style.AlignConsecutiveDeclarations)
     return;
 
   // FIXME: Currently we don't handle properly the PointerAlignment: Right
   // The * and & are not aligned and are left dangling. Something has to be done
   // about it, but it raises the question of alignment of code like:
   //   const char* const* v1;
   //   float const* v2;
   //   SomeVeryLongType const& v3;
 
   AlignTokens(Style,
               [](Change const &C) {
                 return C.Tok->isOneOf(TT_StartOfName,
-                                      TT_FunctionDeclarationName);
+                                      TT_FunctionDeclarationName,
+                                      tok::kw_operator);
               },
-              Changes);
+              Changes, 0);
 }
 
 void WhitespaceManager::alignTrailingComments() {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
   unsigned StartOfSequence = 0;
   bool BreakBeforeNext = false;
   unsigned Newlines = 0;

unittests/Format/FormatTest.cpp

verifyFormat("auto lambda = []() {\n"
                "int i  = 0;\n"
                "auto v = type{\n"
                "    i = 1,   //\n"
                "    (i = 2), //\n"
                "    i = 3    //\n"
                "};",
                Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
       "int i      = 1;\n"
       "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n"
       "                          loooooooooooooooooooooongParameterB);\n"
-      "int j = 2;",
+      "int j      = 2;",
       Alignment);
 
   verifyFormat("template <typename T, typename T_0 = very_long_type_name_0,\n"
                "          typename B   = very_long_type_name_1,\n"
                "          typename T_2 = very_long_type_name_2>\n"
                "auto foo() {}\n",
                Alignment);
   verifyFormat("int a, b = 1;\n"
EXPECT_EQ("float a = 5;\n"
             "unsigned oneTwoThree = 123;\n"
             "int      oneTwo = 12;",
             format("float    a = 5;\n"
                    "int one = 1;\n"
                    "\n"
                    "unsigned oneTwoThree = 123;\n"
                    "int oneTwo = 12;",
                    Alignment));
+  // Function prototype alignment
+  verifyFormat("int    a();\n"
+               "double b();",
+               Alignment);
+  verifyFormat("int    a(int x);\n"
+               "double b();",
+               Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("int    a(int   x,\n"
+            "         float y);\n"
+            "double b(int    x,\n"
+            "         double y);",
+            format("int a(int x,\n"
+                   " float y);\n"
+                   "double b(int x,\n"
+                   " double y);",
+                   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  // This is the "split function parameter alignment" issue mentioned in
+  // WhitespaceManager.cpp
+  EXPECT_EQ("double a(int x);\n"
+            "int    b(int    x,\n"
+            "         double y);",
+            format("double a(int x);\n"
+                   "int b(int x,\n"
+                   " double y);",
+                   Alignment));
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("int    a(int x, void (*fp)(int y));\n"
+               "double b();",
+               Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+               "  int b  = 2;\n"
+               "  int cc = 3;\n"
+               "}",
+               Alignment);
   verifyFormat("float      something = 2000;\n"
                "double     another   = 911;\n"
                "int        i = 1, j = 10;\n"
                "const int *oneMore = 1;\n"
                "unsigned   i       = 2;",
                Alignment);
   verifyFormat("int      oneTwoThree = {0}; // comment\n"
                "unsigned oneTwo      = 0;   // comment",
                Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+               "  j = i;\n"
+               "double x = 1;\n",
+               Alignment);
+  verifyFormat("if (int i = 0)\n"
+               "  j = i;\n"
+               "double x = 1;\n",
+               Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+               "  long long int foo();\n"
+               "  int           operator[](int a);\n"
+               "  double        bar();\n"
+               "};\n",
+               Alignment);
+  verifyFormat("struct test {\n"
+               "  long long int foo();\n"
+               "  int           operator()(int a);\n"
+               "  double        bar();\n"
+               "};\n",
+               Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
             "  int const i   = 1;\n"
             "  int *     j   = 2;\n"
             "  int       big = 10000;\n"
             "\n"
             "  unsigned oneTwoThree = 123;\n"
             "  int      oneTwo      = 12;\n"
             "  method();\n"
verifyFormat("auto lambda = []() {\n"
                "auto  v  = type{\n"
                "    i = 1,   //\n"
                "    (i = 2), //\n"
                "    i = 3    //\n"
                "};",
                Alignment);
   Alignment.AlignConsecutiveAssignments = false;
 
-  // FIXME: Should align all three declarations
   verifyFormat(
       "int      i = 1;\n"
       "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n"
       "                          loooooooooooooooooooooongParameterB);\n"
-      "int j = 2;",
+      "int      j = 2;",
       Alignment);
 
   // Test interactions with ColumnLimit and AlignConsecutiveAssignments:
   // We expect declarations and assignments to align, as long as it doesn't
-  // exceed the column limit, starting a new alignemnt sequence whenever it
+  // exceed the column limit, starting a new alignment sequence whenever it
   // happens.
   Alignment.AlignConsecutiveAssignments = true;
   Alignment.ColumnLimit = 30;
   verifyFormat("float    ii              = 1;\n"
                "unsigned j               = 2;\n"
                "int someVerylongVariable = 1;\n"
                "AnotherLongType  ll = 123456;\n"
                "VeryVeryLongType k  = 2;\n"