Fix alignment of trailing block comments.

[alexfh]

Summary

This patch ensures that the lines of the block comments retain relative
column offsets. In order to do this WhitespaceManager::Changes representing
continuation of block comments keep a pointer on the change representing the
whitespace change before the block comment, and a relative column offset to this
change, so that the correct column can be reconstructed at the end of alignment
process.

Fixes http://llvm.org/PR19325

[djasper]

Two nits, otherwise looks good (assuming the tests pass ;-) ).

lib/Format/WhitespaceManager.cpp

136

I think this might actually be shorter with if .. else ..

lib/Format/WhitespaceManager.h

147

Please add a comment.

lib/Format/BreakableToken.h

private:
   // LeadingWhitespace[i] is the number of characters regarded as whitespace in
   // front of Lines[i]. Note that this can include "* " sequences, which we
   // regard as whitespace when all lines have a "*" prefix.
   SmallVector<unsigned, 16> LeadingWhitespace;
 
   // StartOfLineColumn[i] is the target column at which Line[i] should be.
   // Note that this excludes a leading "* " or "*" in case all lines have
   // a "*" prefix.
-  SmallVector<unsigned, 16> StartOfLineColumn;
+  SmallVector<int, 16> StartOfLineColumn;
 
   // The column at which the text of a broken line should start.
   // Note that an optional decoration would go before that column.
   // IndentAtLineBreak is a uniform position for all lines in a block comment,
   // regardless of their relative position.
   // FIXME: Revisit the decision to do this; the main reason was to support
   // patterns like
   // /**************//**

lib/Format/BreakableToken.cpp

void BreakableBlockComment::adjustWhitespace(unsigned LineIndex,
   Lines[LineIndex - 1] = Lines[LineIndex - 1].substr(0, EndOfPreviousLine);
   Lines[LineIndex] = Lines[LineIndex].substr(StartOfLine);
   // Adjust LeadingWhitespace to account all whitespace between the lines
   // to the current line.
   LeadingWhitespace[LineIndex] =
       Lines[LineIndex].begin() - Lines[LineIndex - 1].end();
 
   // Adjust the start column uniformly across all lines.
-  StartOfLineColumn[LineIndex] = std::max<int>(
-      0,
+  StartOfLineColumn[LineIndex] =
       encoding::columnWidthWithTabs(Whitespace, 0, Style.TabWidth, Encoding) +
-          IndentDelta);
+      IndentDelta;
 }
 
 unsigned BreakableBlockComment::getLineCount() const { return Lines.size(); }
 
 unsigned BreakableBlockComment::getLineLengthAfterSplit(
     unsigned LineIndex, unsigned Offset, StringRef::size_type Length) const {
   unsigned ContentStartColumn = getContentStartColumn(LineIndex, Offset);
   return ContentStartColumn +
if (StartOfLineColumn[LineIndex] == 1) {
       // This line starts immediately after the decorating *.
       Prefix = Prefix.substr(0, 1);
     }
   }
 
   unsigned WhitespaceOffsetInToken = Lines[LineIndex].data() -
                                      Tok.TokenText.data() -
                                      LeadingWhitespace[LineIndex];
-  assert(StartOfLineColumn[LineIndex] >= Prefix.size());
   Whitespaces.replaceWhitespaceInToken(
       Tok, WhitespaceOffsetInToken, LeadingWhitespace[LineIndex], "", Prefix,
       InPPDirective, 1, IndentLevel,
       StartOfLineColumn[LineIndex] - Prefix.size());
 }
 
 unsigned
 BreakableBlockComment::getContentStartColumn(unsigned LineIndex,
                                              unsigned TailOffset) const {
   // If we break, we always break at the predefined indent.
   if (TailOffset != 0)
     return IndentAtLineBreak;
-  return StartOfLineColumn[LineIndex];
+  return std::max(0, StartOfLineColumn[LineIndex]);
 }
 
 } // namespace format
 } // namespace clang

lib/Format/WhitespaceManager.h

public:
   void addUntouchableToken(const FormatToken &Tok, bool InPPDirective);
 
   /// \brief Inserts or replaces whitespace in the middle of a token.
   ///
   /// Inserts \p PreviousPostfix, \p Newlines, \p Spaces and \p CurrentPrefix
   /// (in this order) at \p Offset inside \p Tok, replacing \p ReplaceChars
   /// characters.
   ///
+  /// Note: \p Spaces can be negative to retain information about initial
+  /// relative column offset between a line of a block comment and the start of
+  /// the comment. This negative offset may be compensated by trailing comment
+  /// alignment here. In all other cases negative \p Spaces will be truncated to
+  /// 0.
+  ///
   /// When \p InPPDirective is true, escaped newlines are inserted. \p Spaces is
   /// used to align backslashes correctly.
   void replaceWhitespaceInToken(const FormatToken &Tok, unsigned Offset,
                                 unsigned ReplaceChars,
                                 StringRef PreviousPostfix,
                                 StringRef CurrentPrefix, bool InPPDirective,
                                 unsigned Newlines, unsigned IndentLevel,
-                                unsigned Spaces);
+                                int Spaces);
 
   /// \brief Returns all the \c Replacements created during formatting.
   const tooling::Replacements &generateReplacements();
 
 private:
   /// \brief Represents a change before a token, a break inside a token,
   /// or the layout of an unchanged token (or whitespace within).
   struct Change {
struct Change {
     /// The generated \c Change will replace the characters at
     /// \p OriginalWhitespaceRange with a concatenation of
     /// \p PreviousLinePostfix, \p NewlinesBefore line breaks, \p Spaces spaces
     /// and \p CurrentLinePrefix.
     ///
     /// \p StartOfTokenColumn and \p InPPDirective will be used to lay out
     /// trailing comments and escaped newlines.
     Change(bool CreateReplacement, const SourceRange &OriginalWhitespaceRange,
-           unsigned IndentLevel, unsigned Spaces, unsigned StartOfTokenColumn,
+           unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
            unsigned NewlinesBefore, StringRef PreviousLinePostfix,
            StringRef CurrentLinePrefix, tok::TokenKind Kind,
            bool ContinuesPPDirective);
 
     bool CreateReplacement;
     // Changes might be in the middle of a token, so we cannot just keep the
     // FormatToken around to query its information.
     SourceRange OriginalWhitespaceRange;
struct Change {
 
     // The number of nested blocks the token is in. This is used to add tabs
     // only for the indentation, and not for alignment, when
     // UseTab = US_ForIndentation.
     unsigned IndentLevel;
 
     // The number of spaces in front of the token or broken part of the token.
     // This will be adapted when aligning tokens.
-    unsigned Spaces;
+    int Spaces;
 
     // \c IsTrailingComment, \c TokenLength, \c PreviousEndOfTokenColumn and
     // \c EscapedNewlineColumn will be calculated in
     // \c calculateLineBreakInformation.
     bool IsTrailingComment;
     unsigned TokenLength;
     unsigned PreviousEndOfTokenColumn;
     unsigned EscapedNewlineColumn;
+
+    const Change *StartOfBlockComment;
+    int IndentationOffset;
   };
 
   /// \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 trailing comments over all \c Changes.

lib/Format/WhitespaceManager.cpp

WhitespaceManager::Change::IsBeforeInFile::operator()(const Change &C1,
                                                       const Change &C2) const {
   return SourceMgr.isBeforeInTranslationUnit(
       C1.OriginalWhitespaceRange.getBegin(),
       C2.OriginalWhitespaceRange.getBegin());
 }
 
 WhitespaceManager::Change::Change(
     bool CreateReplacement, const SourceRange &OriginalWhitespaceRange,
-    unsigned IndentLevel, unsigned Spaces, unsigned StartOfTokenColumn,
+    unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
     unsigned NewlinesBefore, StringRef PreviousLinePostfix,
     StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective)
     : CreateReplacement(CreateReplacement),
       OriginalWhitespaceRange(OriginalWhitespaceRange),
       StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
       PreviousLinePostfix(PreviousLinePostfix),
       CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
       ContinuesPPDirective(ContinuesPPDirective), IndentLevel(IndentLevel),
Changes.push_back(Change(false, Tok.WhitespaceRange, /*IndentLevel=*/0,
                            /*Spaces=*/0, Tok.OriginalColumn, Tok.NewlinesBefore,
                            "", "", Tok.Tok.getKind(),
                            InPPDirective && !Tok.IsFirst));
 }
 
 void WhitespaceManager::replaceWhitespaceInToken(
     const FormatToken &Tok, unsigned Offset, unsigned ReplaceChars,
     StringRef PreviousPostfix, StringRef CurrentPrefix, bool InPPDirective,
-    unsigned Newlines, unsigned IndentLevel, unsigned Spaces) {
+    unsigned Newlines, unsigned IndentLevel, int Spaces) {
   if (Tok.Finalized)
     return;
+  SourceLocation Start = Tok.getStartOfNonWhitespace().getLocWithOffset(Offset);
   Changes.push_back(Change(
-      true, SourceRange(Tok.getStartOfNonWhitespace().getLocWithOffset(Offset),
-                        Tok.getStartOfNonWhitespace().getLocWithOffset(
-                            Offset + ReplaceChars)),
-      IndentLevel, Spaces, Spaces, Newlines, PreviousPostfix, CurrentPrefix,
+      true, SourceRange(Start, Start.getLocWithOffset(ReplaceChars)),
+      IndentLevel, Spaces, std::max(0, Spaces), Newlines, PreviousPostfix,
+      CurrentPrefix,
       // If we don't add a newline this change doesn't start a comment. Thus,
       // when we align line comments, we don't need to treat this change as one.
       // FIXME: We still need to take this change in account to properly
       // calculate the new length of the comment and to calculate the changes
       // for which to do the alignment when aligning comments.
       Tok.Type == TT_LineComment && Newlines > 0 ? tok::comment : tok::unknown,
       InPPDirective && !Tok.IsFirst));
 }
for (unsigned i = 1, e = Changes.size(); i != e; ++i) {
     Changes[i - 1].IsTrailingComment =
         (Changes[i].NewlinesBefore > 0 || Changes[i].Kind == tok::eof) &&
         Changes[i - 1].Kind == tok::comment;
   }
   // FIXME: The last token is currently not always an eof token; in those
   // cases, setting TokenLength of the last token to 0 is wrong.
   Changes.back().TokenLength = 0;
   Changes.back().IsTrailingComment = Changes.back().Kind == tok::comment;
+
+  const WhitespaceManager::Change *LastBlockComment = nullptr;
+  for (auto &Change : Changes) {
+    Change.StartOfBlockComment = nullptr;
+    if (Change.Kind == tok::comment) {
+      LastBlockComment = &Change;
+    } else if (Change.Kind == tok::unknown) {
+      Change.StartOfBlockComment = LastBlockComment;
+    } else {
+      LastBlockComment = nullptr;
+    }
+    Change.IndentationOffset =
+        Change.StartOfBlockComment
+            ? Change.StartOfTokenColumn -
+                  Change.StartOfBlockComment->StartOfTokenColumn
+            : 0;
+  }
 }
 
 void WhitespaceManager::alignTrailingComments() {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
   unsigned StartOfSequence = 0;
   bool BreakBeforeNext = false;
   unsigned Newlines = 0;
   for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
+    if (Changes[i].StartOfBlockComment)
+      continue;
+    Newlines += Changes[i].NewlinesBefore;
+    if (!Changes[i].IsTrailingComment)
+      continue;
+
     unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
     // FIXME: Correctly handle ChangeMaxColumn in PP directives.
     unsigned ChangeMaxColumn = Style.ColumnLimit - Changes[i].TokenLength;
-    Newlines += Changes[i].NewlinesBefore;
-    if (Changes[i].IsTrailingComment) {
-      // If this comment follows an } in column 0, it probably documents the
-      // closing of a namespace and we don't want to align it.
-      bool FollowsRBraceInColumn0 = i > 0 && Changes[i].NewlinesBefore == 0 &&
-                                    Changes[i - 1].Kind == tok::r_brace &&
-                                    Changes[i - 1].StartOfTokenColumn == 0;
-      bool WasAlignedWithStartOfNextLine = false;
-      if (Changes[i].NewlinesBefore == 1) { // A comment on its own line.
-        for (unsigned j = i + 1; j != e; ++j) {
-          if (Changes[j].Kind != tok::comment) { // Skip over comments.
-            // The start of the next token was previously aligned with the
-            // start of this comment.
-            WasAlignedWithStartOfNextLine =
-                (SourceMgr.getSpellingColumnNumber(
-                     Changes[i].OriginalWhitespaceRange.getEnd()) ==
-                 SourceMgr.getSpellingColumnNumber(
-                     Changes[j].OriginalWhitespaceRange.getEnd()));
-            break;
-          }
-        }
-      }
-      if (!Style.AlignTrailingComments || FollowsRBraceInColumn0) {
-        alignTrailingComments(StartOfSequence, i, MinColumn);
-        MinColumn = ChangeMinColumn;
-        MaxColumn = ChangeMinColumn;
-        StartOfSequence = i;
-      } else if (BreakBeforeNext || Newlines > 1 ||
-                 (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn) ||
-                 // Break the comment sequence if the previous line did not end
-                 // in a trailing comment.
-                 (Changes[i].NewlinesBefore == 1 && i > 0 &&
-                  !Changes[i - 1].IsTrailingComment) ||
-                 WasAlignedWithStartOfNextLine) {
-        alignTrailingComments(StartOfSequence, i, MinColumn);
-        MinColumn = ChangeMinColumn;
-        MaxColumn = ChangeMaxColumn;
-        StartOfSequence = i;
-      } else {
-        MinColumn = std::max(MinColumn, ChangeMinColumn);
-        MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
-      }
-      BreakBeforeNext =
-          (i == 0) || (Changes[i].NewlinesBefore > 1) ||
-          // Never start a sequence with a comment at the beginning of
-          // the line.
-          (Changes[i].NewlinesBefore == 1 && StartOfSequence == i);
-      Newlines = 0;
-    }
-  }
   alignTrailingComments(StartOfSequence, Changes.size(), MinColumn);
 }
 
 void WhitespaceManager::alignTrailingComments(unsigned Start, unsigned End,
                                               unsigned Column) {
   for (unsigned i = Start; i != End; ++i) {
+    int Shift = 0;
     if (Changes[i].IsTrailingComment) {
-      assert(Column >= Changes[i].StartOfTokenColumn);
-      Changes[i].Spaces += Column - Changes[i].StartOfTokenColumn;
-      if (i + 1 != End) {
-        Changes[i + 1].PreviousEndOfTokenColumn +=
-            Column - Changes[i].StartOfTokenColumn;
-      }
-      Changes[i].StartOfTokenColumn = Column;
+      Shift = Column - Changes[i].StartOfTokenColumn;
     }
+    if (Changes[i].StartOfBlockComment) {
+      Shift = Changes[i].IndentationOffset +
+              Changes[i].StartOfBlockComment->StartOfTokenColumn -
+              Changes[i].StartOfTokenColumn;
+    }
+    assert(Shift >= 0);
+    Changes[i].Spaces += Shift;
+    if (i + 1 != End)
+      Changes[i + 1].PreviousEndOfTokenColumn += Shift;
+    Changes[i].StartOfTokenColumn += Shift;
   }
 }
 
 void WhitespaceManager::alignEscapedNewlines() {
   unsigned MaxEndOfLine =
       Style.AlignEscapedNewlinesLeft ? 0 : Style.ColumnLimit;
   unsigned StartOfMacro = 0;
   for (unsigned i = 1, e = Changes.size(); i < e; ++i) {
for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
     const Change &C = Changes[i];
     if (C.CreateReplacement) {
       std::string ReplacementText = C.PreviousLinePostfix;
       if (C.ContinuesPPDirective)
         appendNewlineText(ReplacementText, C.NewlinesBefore,
                           C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
       else
         appendNewlineText(ReplacementText, C.NewlinesBefore);
-      appendIndentText(ReplacementText, C.IndentLevel, C.Spaces,
-                       C.StartOfTokenColumn - C.Spaces);
+      appendIndentText(ReplacementText, C.IndentLevel, std::max(0, C.Spaces),
+                       C.StartOfTokenColumn - std::max(0, C.Spaces));
       ReplacementText.append(C.CurrentLinePrefix);
       storeReplacement(C.OriginalWhitespaceRange, ReplacementText);
     }
   }
 }
 
 void WhitespaceManager::storeReplacement(const SourceRange &Range,
                                          StringRef Text) {

unittests/Format/FormatTest.cpp

EXPECT_EQ("/*\n"
                    " */"));
 
   EXPECT_EQ("int i; /* Comment with empty...\n"
             "        *\n"
             "        * line. */",
             format("int i; /* Comment with empty...\n"
                    "        *\n"
                    "        * line. */"));
+  EXPECT_EQ("int foobar = 0; /* comment */\n"
+            "int bar = 0;    /* multiline\n"
+            "                   comment 1 */\n"
+            "int baz = 0;    /* multiline\n"
+            "                   comment 2 */\n"
+            "int bzz = 0;    /* multiline\n"
+            "                   comment 3 */",
+            format("int foobar = 0; /* comment */\n"
+                   "int bar = 0;    /* multiline\n"
+                   "                   comment 1 */\n"
+                   "int baz = 0; /* multiline\n"
+                   "                comment 2 */\n"
+                   "int bzz = 0;         /* multiline\n"
+                   "                        comment 3 */"));
+  EXPECT_EQ("int foobar = 0; /* comment */\n"
+            "int bar = 0;    /* multiline\n"
+            "   comment */\n"
+            "int baz = 0;    /* multiline\n"
+            "comment */",
+            format("int foobar = 0; /* comment */\n"
+                   "int bar = 0; /* multiline\n"
+                   "comment */\n"
+                   "int baz = 0;        /* multiline\n"
+                   "comment */"));
 }
 
 TEST_F(FormatTest, CorrectlyHandlesLengthOfBlockComments) {
   EXPECT_EQ("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
             "              aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */",
             format("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                    "              aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */"));
   EXPECT_EQ(