From b77d741691a2775b5c31e29f021203cc659c26df Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Fri, 6 Sep 2013 07:54:20 +0000 Subject: [PATCH] clang-format: Fix comment formatting bugs in nested blocks. This fixes two issues: 1) The indent of a line comment was not adapted to the subsequent statement as it would be outside of a nested block. 2) A missing DryRun flag caused actualy breaks to be inserted in overly long comments while trying to come up with the best line breaking decisions. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190123 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Format/ContinuationIndenter.cpp | 6 ++-- lib/Format/ContinuationIndenter.h | 3 +- lib/Format/Format.cpp | 52 ++++++++++++----------------- lib/Format/TokenAnnotator.cpp | 25 +++++++++++--- lib/Format/TokenAnnotator.h | 7 +++- unittests/Format/FormatTest.cpp | 29 ++++++++++++++++ 6 files changed, 83 insertions(+), 39 deletions(-) diff --git a/lib/Format/ContinuationIndenter.cpp b/lib/Format/ContinuationIndenter.cpp index 9e84ea770f..d48cb23da9 100644 --- a/lib/Format/ContinuationIndenter.cpp +++ b/lib/Format/ContinuationIndenter.cpp @@ -63,7 +63,8 @@ ContinuationIndenter::ContinuationIndenter(const FormatStyle &Style, BinPackInconclusiveFunctions(BinPackInconclusiveFunctions) {} LineState ContinuationIndenter::getInitialState(unsigned FirstIndent, - const AnnotatedLine *Line) { + const AnnotatedLine *Line, + bool DryRun) { LineState State; State.FirstIndent = FirstIndent; State.Column = FirstIndent; @@ -80,8 +81,7 @@ LineState ContinuationIndenter::getInitialState(unsigned FirstIndent, State.IgnoreStackForComparison = false; // The first token has already been indented and thus consumed. - moveStateToNextToken(State, /*DryRun=*/false, - /*Newline=*/false); + moveStateToNextToken(State, DryRun, /*Newline=*/false); return State; } diff --git a/lib/Format/ContinuationIndenter.h b/lib/Format/ContinuationIndenter.h index 5d3da4ccf8..ef698a7a52 100644 --- a/lib/Format/ContinuationIndenter.h +++ b/lib/Format/ContinuationIndenter.h @@ -41,7 +41,8 @@ public: /// \brief Get the initial state, i.e. the state after placing \p Line's /// first token at \p FirstIndent. - LineState getInitialState(unsigned FirstIndent, const AnnotatedLine *Line); + LineState getInitialState(unsigned FirstIndent, const AnnotatedLine *Line, + bool DryRun); // FIXME: canBreak and mustBreak aren't strictly indentation-related. Find a // better home. diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp index 02adc5acd6..7d4048f104 100644 --- a/lib/Format/Format.cpp +++ b/lib/Format/Format.cpp @@ -328,7 +328,8 @@ public: /// \brief Formats the line starting at \p State, simply keeping all of the /// input's line breaking decisions. void format(unsigned FirstIndent, const AnnotatedLine *Line) { - LineState State = Indenter->getInitialState(FirstIndent, Line); + LineState State = + Indenter->getInitialState(FirstIndent, Line, /*DryRun=*/false); while (State.NextToken != NULL) { bool Newline = Indenter->mustBreak(State) || @@ -353,7 +354,7 @@ public: /// /// If \p DryRun is \c false, directly applies the changes. unsigned format(unsigned FirstIndent, bool DryRun = false) { - LineState State = Indenter->getInitialState(FirstIndent, &Line); + LineState State = Indenter->getInitialState(FirstIndent, &Line, DryRun); // If the ObjC method declaration does not fit on a line, we should format // it with one arg per line. @@ -757,25 +758,14 @@ public: Annotator.calculateFormattingInformation(*AnnotatedLines[i]); } - // Adapt level to the next line if this is a comment. - // FIXME: Can/should this be done in the UnwrappedLineParser? - const AnnotatedLine *NextNonCommentLine = NULL; - for (unsigned i = AnnotatedLines.size() - 1; i > 0; --i) { - if (NextNonCommentLine && AnnotatedLines[i]->First->is(tok::comment) && - !AnnotatedLines[i]->First->Next) - AnnotatedLines[i]->Level = NextNonCommentLine->Level; - else - NextNonCommentLine = AnnotatedLines[i]->First->isNot(tok::r_brace) - ? AnnotatedLines[i] - : NULL; - } + Annotator.setCommentLineLevels(AnnotatedLines); std::vector IndentForLevel; bool PreviousLineWasTouched = false; const FormatToken *PreviousLineLastToken = 0; bool FormatPPDirective = false; - for (std::vector::iterator I = AnnotatedLines.begin(), - E = AnnotatedLines.end(); + for (SmallVectorImpl::iterator I = AnnotatedLines.begin(), + E = AnnotatedLines.end(); I != E; ++I) { const AnnotatedLine &TheLine = **I; const FormatToken *FirstTok = TheLine.First; @@ -827,7 +817,8 @@ public: ColumnLimit = getColumnLimit(TheLine.InPPDirective); if (TheLine.Last->TotalLength + Indent <= ColumnLimit) { - LineState State = Indenter.getInitialState(Indent, &TheLine); + LineState State = + Indenter.getInitialState(Indent, &TheLine, /*DryRun=*/false); while (State.NextToken != NULL) Indenter.addTokenToState(State, false, false); } else if (Style.ColumnLimit == 0) { @@ -954,8 +945,8 @@ private: /// This will change \c Line and \c AnnotatedLine to contain the merged line, /// if possible; note that \c I will be incremented when lines are merged. void tryFitMultipleLinesInOne(unsigned Indent, - std::vector::iterator &I, - std::vector::iterator E) { + SmallVectorImpl::iterator &I, + SmallVectorImpl::iterator E) { // We can never merge stuff if there are trailing line comments. AnnotatedLine *TheLine = *I; if (TheLine->Last->Type == TT_LineComment) @@ -988,8 +979,8 @@ private: } } - void tryMergeSimplePPDirective(std::vector::iterator &I, - std::vector::iterator E, + void tryMergeSimplePPDirective(SmallVectorImpl::iterator &I, + SmallVectorImpl::iterator E, unsigned Limit) { if (Limit == 0) return; @@ -1004,9 +995,10 @@ private: join(Line, **(++I)); } - void tryMergeSimpleControlStatement(std::vector::iterator &I, - std::vector::iterator E, - unsigned Limit) { + void + tryMergeSimpleControlStatement(SmallVectorImpl::iterator &I, + SmallVectorImpl::iterator E, + unsigned Limit) { if (Limit == 0) return; if (Style.BreakBeforeBraces == FormatStyle::BS_Allman && @@ -1031,8 +1023,8 @@ private: join(Line, **(++I)); } - void tryMergeSimpleBlock(std::vector::iterator &I, - std::vector::iterator E, + void tryMergeSimpleBlock(SmallVectorImpl::iterator &I, + SmallVectorImpl::iterator E, unsigned Limit) { // No merging if the brace already is on the next line. if (Style.BreakBeforeBraces != FormatStyle::BS_Attach) @@ -1086,7 +1078,7 @@ private: } } - bool nextTwoLinesFitInto(std::vector::iterator I, + bool nextTwoLinesFitInto(SmallVectorImpl::iterator I, unsigned Limit) { return 1 + (*(I + 1))->Last->TotalLength + 1 + (*(I + 2))->Last->TotalLength <= @@ -1126,8 +1118,8 @@ private: return touchesRanges(LineRange); } - bool touchesPPDirective(std::vector::iterator I, - std::vector::iterator E) { + bool touchesPPDirective(SmallVectorImpl::iterator I, + SmallVectorImpl::iterator E) { for (; I != E; ++I) { if ((*I)->First->HasUnescapedNewline) return false; @@ -1186,7 +1178,7 @@ private: SourceManager &SourceMgr; WhitespaceManager Whitespaces; std::vector Ranges; - std::vector AnnotatedLines; + SmallVector AnnotatedLines; encoding::Encoding Encoding; bool BinPackInconclusiveFunctions; diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp index 46baab4aab..2f3be557ce 100644 --- a/lib/Format/TokenAnnotator.cpp +++ b/lib/Format/TokenAnnotator.cpp @@ -974,9 +974,26 @@ private: } // end anonymous namespace +void +TokenAnnotator::setCommentLineLevels(SmallVectorImpl &Lines) { + if (Lines.empty()) + return; + + const AnnotatedLine *NextNonCommentLine = NULL; + for (unsigned i = Lines.size() - 1; i > 0; --i) { + if (NextNonCommentLine && Lines[i]->First->is(tok::comment) && + !Lines[i]->First->Next) + Lines[i]->Level = NextNonCommentLine->Level; + else + NextNonCommentLine = + Lines[i]->First->isNot(tok::r_brace) ? Lines[i] : NULL; + } +} + void TokenAnnotator::annotate(AnnotatedLine &Line) { - for (std::vector::iterator I = Line.Children.begin(), - E = Line.Children.end(); + setCommentLineLevels(Line.Children); + for (SmallVectorImpl::iterator I = Line.Children.begin(), + E = Line.Children.end(); I != E; ++I) { annotate(**I); } @@ -1056,8 +1073,8 @@ void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) { DEBUG({ printDebugInfo(Line); }); - for (std::vector::iterator I = Line.Children.begin(), - E = Line.Children.end(); + for (SmallVectorImpl::iterator I = Line.Children.begin(), + E = Line.Children.end(); I != E; ++I) { calculateFormattingInformation(**I); } diff --git a/lib/Format/TokenAnnotator.h b/lib/Format/TokenAnnotator.h index 5546cdc49d..06f335215d 100644 --- a/lib/Format/TokenAnnotator.h +++ b/lib/Format/TokenAnnotator.h @@ -71,7 +71,7 @@ public: FormatToken *First; FormatToken *Last; - std::vector Children; + SmallVector Children; LineType Type; unsigned Level; @@ -93,6 +93,11 @@ public: TokenAnnotator(const FormatStyle &Style, IdentifierInfo &Ident_in) : Style(Style), Ident_in(Ident_in) {} + /// \brief Adapts the indent levels of comment lines to the indent of the + /// subsequent line. + // FIXME: Can/should this be done in the UnwrappedLineParser? + void setCommentLineLevels(SmallVectorImpl &Lines); + void annotate(AnnotatedLine &Line); void calculateFormattingInformation(AnnotatedLine &Line); diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp index 6d953eb301..3eaa69a8b6 100644 --- a/unittests/Format/FormatTest.cpp +++ b/unittests/Format/FormatTest.cpp @@ -564,6 +564,17 @@ TEST_F(FormatTest, FormatsSwitchStatement) { " }\n" "#undef OPERATION_CASE\n" "}"); + verifyFormat("DEBUG({\n" + " switch (x) {\n" + " case A:\n" + " f();\n" + " break;\n" + " // On B:\n" + " case B:\n" + " g();\n" + " break;\n" + " }\n" + "});"); } TEST_F(FormatTest, FormatsLabels) { @@ -2241,6 +2252,24 @@ TEST_F(FormatTest, LayoutNestedBlocks) { " for (int i = 0; i < 10; ++i)\n" " return;\n" "}"); + verifyFormat("call(parameter, {\n" + " something();\n" + " // Comment using all columns.\n" + " somethingelse();\n" + "});", + getLLVMStyleWithColumns(40)); + EXPECT_EQ("call(parameter, {\n" + " something();\n" + " // Comment too\n" + " // looooooooooong.\n" + " somethingElse();\n" + "});", + format("call(parameter, {\n" + " something();\n" + " // Comment too looooooooooong.\n" + " somethingElse();\n" + "});", + getLLVMStyleWithColumns(29))); } TEST_F(FormatTest, PutEmptyBlocksIntoOneLine) { -- 2.40.0