From 67d9c8c87ef4451214211ae8bb44f84ec7713a4b Mon Sep 17 00:00:00 2001 From: Alexander Kornienko Date: Thu, 17 Apr 2014 16:12:46 +0000 Subject: [PATCH] Fix alignment of trailing block comments. 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 Reviewers: djasper Reviewed By: djasper CC: cfe-commits, klimek Differential Revision: http://reviews.llvm.org/D3408 llvm-svn: 206472 --- clang/lib/Format/BreakableToken.cpp | 8 +- clang/lib/Format/BreakableToken.h | 2 +- clang/lib/Format/WhitespaceManager.cpp | 146 ++++++++++++++----------- clang/lib/Format/WhitespaceManager.h | 26 ++++- clang/unittests/Format/FormatTest.cpp | 24 ++++ 5 files changed, 136 insertions(+), 70 deletions(-) diff --git a/clang/lib/Format/BreakableToken.cpp b/clang/lib/Format/BreakableToken.cpp index d43a774c9cb0..40b53f3026ed 100644 --- a/clang/lib/Format/BreakableToken.cpp +++ b/clang/lib/Format/BreakableToken.cpp @@ -350,10 +350,9 @@ void BreakableBlockComment::adjustWhitespace(unsigned LineIndex, Lines[LineIndex].begin() - Lines[LineIndex - 1].end(); // Adjust the start column uniformly across all lines. - StartOfLineColumn[LineIndex] = std::max( - 0, + StartOfLineColumn[LineIndex] = encoding::columnWidthWithTabs(Whitespace, 0, Style.TabWidth, Encoding) + - IndentDelta); + IndentDelta; } unsigned BreakableBlockComment::getLineCount() const { return Lines.size(); } @@ -437,7 +436,6 @@ BreakableBlockComment::replaceWhitespaceBefore(unsigned LineIndex, 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, @@ -450,7 +448,7 @@ BreakableBlockComment::getContentStartColumn(unsigned LineIndex, // 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 diff --git a/clang/lib/Format/BreakableToken.h b/clang/lib/Format/BreakableToken.h index e294a5058010..72bb1e4404ff 100644 --- a/clang/lib/Format/BreakableToken.h +++ b/clang/lib/Format/BreakableToken.h @@ -212,7 +212,7 @@ private: // 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 StartOfLineColumn; + SmallVector StartOfLineColumn; // The column at which the text of a broken line should start. // Note that an optional decoration would go before that column. diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index 4c393ed43976..cea479974317 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -28,7 +28,7 @@ WhitespaceManager::Change::IsBeforeInFile::operator()(const Change &C1, 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), @@ -69,14 +69,14 @@ void WhitespaceManager::addUntouchableToken(const FormatToken &Tok, 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 @@ -122,6 +122,22 @@ void WhitespaceManager::calculateLineBreakInformation() { // 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; + Change.IndentationOffset = 0; + if (Change.Kind == tok::comment) { + LastBlockComment = &Change; + } else if (Change.Kind == tok::unknown) { + if ((Change.StartOfBlockComment = LastBlockComment)) + Change.IndentationOffset = + Change.StartOfTokenColumn - + Change.StartOfBlockComment->StartOfTokenColumn; + } else { + LastBlockComment = nullptr; + } + } } void WhitespaceManager::alignTrailingComments() { @@ -131,58 +147,61 @@ void WhitespaceManager::alignTrailingComments() { 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 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; } + 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); } @@ -190,15 +209,20 @@ void WhitespaceManager::alignTrailingComments() { 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; } } @@ -245,8 +269,8 @@ void WhitespaceManager::generateChanges() { 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); } diff --git a/clang/lib/Format/WhitespaceManager.h b/clang/lib/Format/WhitespaceManager.h index f94464ff38a1..189b1aefa03f 100644 --- a/clang/lib/Format/WhitespaceManager.h +++ b/clang/lib/Format/WhitespaceManager.h @@ -63,6 +63,12 @@ public: /// (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, @@ -70,7 +76,7 @@ public: 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(); @@ -101,7 +107,7 @@ private: /// \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); @@ -128,7 +134,10 @@ private: // The number of spaces in front of the token or broken part of the token. // This will be adapted when aligning tokens. - unsigned Spaces; + // Can be negative to retain information about the initial relative offset + // of the lines in a block comment. This is used when aligning trailing + // comments. Uncompensated negative offset is truncated to 0. + int Spaces; // \c IsTrailingComment, \c TokenLength, \c PreviousEndOfTokenColumn and // \c EscapedNewlineColumn will be calculated in @@ -137,6 +146,17 @@ private: unsigned TokenLength; unsigned PreviousEndOfTokenColumn; unsigned EscapedNewlineColumn; + + // These fields are used to retain correct relative line indentation in a + // block comment when aligning trailing comments. + // + // 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; }; /// \brief Calculate \c IsTrailingComment, \c TokenLength for the last tokens diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index d3fbb02e5d4c..4c74ed61aac1 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -1013,6 +1013,30 @@ TEST_F(FormatTest, AlignsBlockComments) { 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) {