r190123 - clang-format: Fix comment formatting bugs in nested blocks.
Daniel Jasper
djasper at google.com
Fri Sep 6 00:54:20 PDT 2013
Author: djasper
Date: Fri Sep 6 02:54:20 2013
New Revision: 190123
URL: http://llvm.org/viewvc/llvm-project?rev=190123&view=rev
Log:
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.
Modified:
cfe/trunk/lib/Format/ContinuationIndenter.cpp
cfe/trunk/lib/Format/ContinuationIndenter.h
cfe/trunk/lib/Format/Format.cpp
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/lib/Format/TokenAnnotator.h
cfe/trunk/unittests/Format/FormatTest.cpp
Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Fri Sep 6 02:54:20 2013
@@ -63,7 +63,8 @@ ContinuationIndenter::ContinuationIndent
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::getIniti
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;
}
Modified: cfe/trunk/lib/Format/ContinuationIndenter.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/lib/Format/ContinuationIndenter.h (original)
+++ cfe/trunk/lib/Format/ContinuationIndenter.h Fri Sep 6 02:54:20 2013
@@ -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.
Modified: cfe/trunk/lib/Format/Format.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/lib/Format/Format.cpp (original)
+++ cfe/trunk/lib/Format/Format.cpp Fri Sep 6 02:54:20 2013
@@ -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<int> IndentForLevel;
bool PreviousLineWasTouched = false;
const FormatToken *PreviousLineLastToken = 0;
bool FormatPPDirective = false;
- for (std::vector<AnnotatedLine *>::iterator I = AnnotatedLines.begin(),
- E = AnnotatedLines.end();
+ for (SmallVectorImpl<AnnotatedLine *>::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<AnnotatedLine *>::iterator &I,
- std::vector<AnnotatedLine *>::iterator E) {
+ SmallVectorImpl<AnnotatedLine *>::iterator &I,
+ SmallVectorImpl<AnnotatedLine *>::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<AnnotatedLine *>::iterator &I,
- std::vector<AnnotatedLine *>::iterator E,
+ void tryMergeSimplePPDirective(SmallVectorImpl<AnnotatedLine *>::iterator &I,
+ SmallVectorImpl<AnnotatedLine *>::iterator E,
unsigned Limit) {
if (Limit == 0)
return;
@@ -1004,9 +995,10 @@ private:
join(Line, **(++I));
}
- void tryMergeSimpleControlStatement(std::vector<AnnotatedLine *>::iterator &I,
- std::vector<AnnotatedLine *>::iterator E,
- unsigned Limit) {
+ void
+ tryMergeSimpleControlStatement(SmallVectorImpl<AnnotatedLine *>::iterator &I,
+ SmallVectorImpl<AnnotatedLine *>::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<AnnotatedLine *>::iterator &I,
- std::vector<AnnotatedLine *>::iterator E,
+ void tryMergeSimpleBlock(SmallVectorImpl<AnnotatedLine *>::iterator &I,
+ SmallVectorImpl<AnnotatedLine *>::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<AnnotatedLine *>::iterator I,
+ bool nextTwoLinesFitInto(SmallVectorImpl<AnnotatedLine *>::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<AnnotatedLine *>::iterator I,
- std::vector<AnnotatedLine *>::iterator E) {
+ bool touchesPPDirective(SmallVectorImpl<AnnotatedLine *>::iterator I,
+ SmallVectorImpl<AnnotatedLine *>::iterator E) {
for (; I != E; ++I) {
if ((*I)->First->HasUnescapedNewline)
return false;
@@ -1186,7 +1178,7 @@ private:
SourceManager &SourceMgr;
WhitespaceManager Whitespaces;
std::vector<CharSourceRange> Ranges;
- std::vector<AnnotatedLine *> AnnotatedLines;
+ SmallVector<AnnotatedLine *, 16> AnnotatedLines;
encoding::Encoding Encoding;
bool BinPackInconclusiveFunctions;
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Fri Sep 6 02:54:20 2013
@@ -974,9 +974,26 @@ private:
} // end anonymous namespace
+void
+TokenAnnotator::setCommentLineLevels(SmallVectorImpl<AnnotatedLine *> &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<AnnotatedLine *>::iterator I = Line.Children.begin(),
- E = Line.Children.end();
+ setCommentLineLevels(Line.Children);
+ for (SmallVectorImpl<AnnotatedLine *>::iterator I = Line.Children.begin(),
+ E = Line.Children.end();
I != E; ++I) {
annotate(**I);
}
@@ -1056,8 +1073,8 @@ void TokenAnnotator::calculateFormatting
DEBUG({ printDebugInfo(Line); });
- for (std::vector<AnnotatedLine *>::iterator I = Line.Children.begin(),
- E = Line.Children.end();
+ for (SmallVectorImpl<AnnotatedLine *>::iterator I = Line.Children.begin(),
+ E = Line.Children.end();
I != E; ++I) {
calculateFormattingInformation(**I);
}
Modified: cfe/trunk/lib/Format/TokenAnnotator.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.h?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/lib/Format/TokenAnnotator.h (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.h Fri Sep 6 02:54:20 2013
@@ -71,7 +71,7 @@ public:
FormatToken *First;
FormatToken *Last;
- std::vector<AnnotatedLine *> Children;
+ SmallVector<AnnotatedLine *, 0> 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<AnnotatedLine *> &Lines);
+
void annotate(AnnotatedLine &Line);
void calculateFormattingInformation(AnnotatedLine &Line);
Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=190123&r1=190122&r2=190123&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Sep 6 02:54:20 2013
@@ -564,6 +564,17 @@ TEST_F(FormatTest, FormatsSwitchStatemen
" }\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) {
More information about the cfe-commits
mailing list