Author: krasimir Date: Mon Oct 16 02:08:53 2017 New Revision: 315893 URL: http://llvm.org/viewvc/llvm-project?rev=315893&view=rev Log: [clang-format] Break non-trailing comments, try 2
Summary: This patch enables `BreakableToken` to manage the formatting of non-trailing block comments. It is a refinement of https://reviews.llvm.org/D37007. We discovered that the optimizer outsmarts us on cases where breaking the comment costs considerably less than breaking after the comment. This patch addresses this by ensuring that a newline is inserted between a block comment and the next token. Reviewers: djasper Reviewed By: djasper Subscribers: cfe-commits, klimek Differential Revision: https://reviews.llvm.org/D37695 Modified: cfe/trunk/lib/Format/BreakableToken.cpp cfe/trunk/lib/Format/BreakableToken.h cfe/trunk/lib/Format/ContinuationIndenter.cpp cfe/trunk/lib/Format/ContinuationIndenter.h cfe/trunk/unittests/Format/FormatTestComments.cpp cfe/trunk/unittests/Format/FormatTestJS.cpp Modified: cfe/trunk/lib/Format/BreakableToken.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/BreakableToken.cpp?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/lib/Format/BreakableToken.cpp (original) +++ cfe/trunk/lib/Format/BreakableToken.cpp Mon Oct 16 02:08:53 2017 @@ -599,6 +599,12 @@ unsigned BreakableBlockComment::getLineL } } +bool BreakableBlockComment::introducesBreakBefore(unsigned LineIndex) const { + // A break is introduced when we want delimiters on newline. + return LineIndex == 0 && DelimitersOnNewline && + Lines[0].substr(1).find_first_not_of(Blanks) != StringRef::npos; +} + void BreakableBlockComment::replaceWhitespaceBefore( unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit, Split SplitBefore, WhitespaceManager &Whitespaces) { Modified: cfe/trunk/lib/Format/BreakableToken.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/BreakableToken.h?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/lib/Format/BreakableToken.h (original) +++ cfe/trunk/lib/Format/BreakableToken.h Mon Oct 16 02:08:53 2017 @@ -58,6 +58,8 @@ struct FormatStyle; /// operations that might be executed before the main line breaking occurs: /// - getSplitBefore, for finding a split such that the content preceding it /// needs to be specially reflown, +/// - introducesBreakBefore, for checking if reformatting the beginning +/// of the content introduces a line break before it, /// - getLineLengthAfterSplitBefore, for calculating the line length in columns /// of the remainder of the content after the beginning of the content has /// been reformatted, and @@ -135,6 +137,12 @@ public: return Split(StringRef::npos, 0); } + /// \brief Returns if a break before the content at \p LineIndex will be + /// inserted after the whitespace preceding the content has been reformatted. + virtual bool introducesBreakBefore(unsigned LineIndex) const { + return false; + } + /// \brief Returns the number of columns required to format the piece of line /// at \p LineIndex after the content preceding the whitespace range specified /// \p SplitBefore has been reformatted, but before any breaks are made to @@ -339,6 +347,7 @@ public: Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit, llvm::Regex &CommentPragmasRegex) const override; + bool introducesBreakBefore(unsigned LineIndex) const override; unsigned getLineLengthAfterSplitBefore(unsigned LineIndex, unsigned TailOffset, unsigned PreviousEndColumn, Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original) +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Mon Oct 16 02:08:53 2017 @@ -106,6 +106,7 @@ LineState ContinuationIndenter::getIniti /*AvoidBinPacking=*/false, /*NoLineBreak=*/false)); State.LineContainsContinuedForLoopSection = false; + State.NoContinuation = false; State.StartOfStringLiteral = 0; State.StartOfLineLevel = 0; State.LowestLevelOnLine = 0; @@ -322,6 +323,12 @@ bool ContinuationIndenter::mustBreak(con Previous.TokenText == "\'\\n\'")))) return true; + if (Previous.is(TT_BlockComment) && Previous.IsMultiline) + return true; + + if (State.NoContinuation) + return true; + return false; } @@ -331,6 +338,8 @@ unsigned ContinuationIndenter::addTokenT const FormatToken &Current = *State.NextToken; assert(!State.Stack.empty()); + State.NoContinuation = false; + if ((Current.is(TT_ImplicitStringLiteral) && (Current.Previous->Tok.getIdentifierInfo() == nullptr || Current.Previous->Tok.getIdentifierInfo()->getPPKeywordID() == @@ -1286,7 +1295,7 @@ unsigned ContinuationIndenter::breakProt return 0; } } else if (Current.is(TT_BlockComment)) { - if (!Current.isTrailingComment() || !Style.ReflowComments || + if (!Style.ReflowComments || // If a comment token switches formatting, like // /* clang-format on */, we don't want to break it further, // but we may still want to adjust its indentation. @@ -1332,6 +1341,7 @@ unsigned ContinuationIndenter::breakProt ReflowInProgress = SplitBefore.first != StringRef::npos; TailOffset = ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0; + BreakInserted = BreakInserted || Token->introducesBreakBefore(LineIndex); if (!DryRun) Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns, RemainingSpace, SplitBefore, Whitespaces); @@ -1408,6 +1418,9 @@ unsigned ContinuationIndenter::breakProt State.Stack[i].BreakBeforeParameter = true; } + if (Current.is(TT_BlockComment)) + State.NoContinuation = true; + Penalty += Current.isStringLiteral() ? Style.PenaltyBreakString : Style.PenaltyBreakComment; Modified: cfe/trunk/lib/Format/ContinuationIndenter.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/lib/Format/ContinuationIndenter.h (original) +++ cfe/trunk/lib/Format/ContinuationIndenter.h Mon Oct 16 02:08:53 2017 @@ -318,6 +318,9 @@ struct LineState { /// \brief \c true if this line contains a continued for-loop section. bool LineContainsContinuedForLoopSection; + /// \brief \c true if \p NextToken should not continue this line. + bool NoContinuation; + /// \brief The \c NestingLevel at the start of this line. unsigned StartOfLineLevel; @@ -364,6 +367,8 @@ struct LineState { if (LineContainsContinuedForLoopSection != Other.LineContainsContinuedForLoopSection) return LineContainsContinuedForLoopSection; + if (NoContinuation != Other.NoContinuation) + return NoContinuation; if (StartOfLineLevel != Other.StartOfLineLevel) return StartOfLineLevel < Other.StartOfLineLevel; if (LowestLevelOnLine != Other.LowestLevelOnLine) Modified: cfe/trunk/unittests/Format/FormatTestComments.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestComments.cpp?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/unittests/Format/FormatTestComments.cpp (original) +++ cfe/trunk/unittests/Format/FormatTestComments.cpp Mon Oct 16 02:08:53 2017 @@ -2407,6 +2407,57 @@ TEST_F(FormatTestComments, BlockComments getLLVMStyleWithColumns(15))); } +TEST_F(FormatTestComments, BreaksAfterMultilineBlockCommentsInParamLists) { + EXPECT_EQ("a = f(/* long\n" + " long\n" + " */\n" + " a);", + format("a = f(/* long long */ a);", getLLVMStyleWithColumns(15))); + + EXPECT_EQ("a = f(/* long\n" + " long\n" + " */\n" + " a);", + format("a = f(/* long\n" + " long\n" + " */a);", + getLLVMStyleWithColumns(15))); + + EXPECT_EQ("a = f(/* long\n" + " long\n" + " */\n" + " a);", + format("a = f(/* long\n" + " long\n" + " */ a);", + getLLVMStyleWithColumns(15))); + + EXPECT_EQ("a = f(/* long\n" + " long\n" + " */\n" + " (1 + 1));", + format("a = f(/* long\n" + " long\n" + " */ (1 + 1));", + getLLVMStyleWithColumns(15))); + + EXPECT_EQ( + "a = f(a,\n" + " /* long\n" + " long\n" + " */\n" + " b);", + format("a = f(a, /* long long */ b);", getLLVMStyleWithColumns(15))); + + EXPECT_EQ( + "a = f(a,\n" + " /* long\n" + " long\n" + " */\n" + " (1 + 1));", + format("a = f(a, /* long long */ (1 + 1));", getLLVMStyleWithColumns(15))); +} + TEST_F(FormatTestComments, IndentLineCommentsInStartOfBlockAtEndOfFile) { verifyFormat("{\n" " // a\n" @@ -2805,6 +2856,22 @@ TEST_F(FormatTestComments, NoCrush_Bug34 getLLVMStyleWithColumns(80))); // clang-format on } + +TEST_F(FormatTestComments, NonTrailingBlockComments) { + verifyFormat("const /** comment comment */ A = B;", + getLLVMStyleWithColumns(40)); + + verifyFormat("const /** comment comment comment */ A =\n" + " B;", + getLLVMStyleWithColumns(40)); + + EXPECT_EQ("const /** comment comment comment\n" + " comment */\n" + " A = B;", + format("const /** comment comment comment comment */\n" + " A = B;", + getLLVMStyleWithColumns(40))); +} } // end namespace } // end namespace format } // end namespace clang Modified: cfe/trunk/unittests/Format/FormatTestJS.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestJS.cpp?rev=315893&r1=315892&r2=315893&view=diff ============================================================================== --- cfe/trunk/unittests/Format/FormatTestJS.cpp (original) +++ cfe/trunk/unittests/Format/FormatTestJS.cpp Mon Oct 16 02:08:53 2017 @@ -65,6 +65,27 @@ protected: TEST_F(FormatTestJS, BlockComments) { verifyFormat("/* aaaaaaaaaaaaa */ aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n" " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);"); + // Breaks after a single line block comment. + EXPECT_EQ("aaaaa = bbbb.ccccccccccccccc(\n" + " /** @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */\n" + " mediaMessage);", + format("aaaaa = bbbb.ccccccccccccccc(\n" + " /** " + "@type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */ " + "mediaMessage);", + getGoogleJSStyleWithColumns(70))); + // Breaks after a multiline block comment. + EXPECT_EQ( + "aaaaa = bbbb.ccccccccccccccc(\n" + " /**\n" + " * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n" + " */\n" + " mediaMessage);", + format("aaaaa = bbbb.ccccccccccccccc(\n" + " /**\n" + " * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n" + " */ mediaMessage);", + getGoogleJSStyleWithColumns(70))); } TEST_F(FormatTestJS, JSDocComments) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
