From 1321eb593c8e13738dcaf72e409aa466c8cf6463 Mon Sep 17 00:00:00 2001
From: Daniel Jasper <djasper@google.com>
Date: Tue, 18 Dec 2012 21:05:13 +0000
Subject: [PATCH] Better support for constructor initializers.
We used to format initializers like this (with a sort of hacky implementation):
Constructor()
: Val1(A),
Val2(B) {
and now format like this (with a somewhat better solution):
Constructor()
: Val1(A), Val2(B) {
assuming this would not fit on a single line. Also added tests.
As a side effect we now first analyze whether an UnwrappedLine needs to be
split at all. If not, not splitting it is the best solution by definition. As
this should be a very common case in normal code, not exploring the entire
solution space can provide significant speedup.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@170457 91177308-0d34-0410-b5e6-96231b3b80d8
---
lib/Format/Format.cpp | 93 ++++++++++++++++-----------------
unittests/Format/FormatTest.cpp | 52 ++++++++++++++----
2 files changed, 86 insertions(+), 59 deletions(-)
diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp
index 46af06fb419..45bd97d88dc 100644
--- a/lib/Format/Format.cpp
+++ b/lib/Format/Format.cpp
@@ -37,6 +37,7 @@ struct TokenAnnotation {
TT_OverloadedOperator,
TT_PointerOrReference,
TT_ConditionalExpr,
+ TT_CtorInitializerColon,
TT_LineComment,
TT_BlockComment
};
@@ -73,7 +74,6 @@ FormatStyle getGoogleStyle() {
}
struct OptimizationParameters {
- unsigned PenaltyExtraLine;
unsigned PenaltyIndentLevel;
};
@@ -83,13 +83,9 @@ public:
const UnwrappedLine &Line,
const std::vector<TokenAnnotation> &Annotations,
tooling::Replacements &Replaces, bool StructuralError)
- : Style(Style),
- SourceMgr(SourceMgr),
- Line(Line),
- Annotations(Annotations),
- Replaces(Replaces),
+ : Style(Style), SourceMgr(SourceMgr), Line(Line),
+ Annotations(Annotations), Replaces(Replaces),
StructuralError(StructuralError) {
- Parameters.PenaltyExtraLine = 100;
Parameters.PenaltyIndentLevel = 5;
}
@@ -100,8 +96,6 @@ public:
// Initialize state dependent on indent.
IndentState State;
State.Column = Indent;
- State.CtorInitializerOnNewLine = false;
- State.InCtorInitializer = false;
State.ConsumedTokens = 0;
State.Indent.push_back(Indent + 4);
State.LastSpace.push_back(Indent);
@@ -110,11 +104,33 @@ public:
// The first token has already been indented and thus consumed.
moveStateToNextToken(State);
+ // Check whether the UnwrappedLine can be put onto a single line. If so,
+ // this is bound to be the optimal solution (by definition) and we don't
+ // need to analyze the entire solution space.
+ unsigned Columns = State.Column;
+ bool FitsOnALine = true;
+ for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
+ Columns += (Annotations[i].SpaceRequiredBefore ? 1 : 0) +
+ Line.Tokens[i].Tok.getLength();
+ // A special case for the colon of a constructor initializer as this only
+ // needs to be put on a new line if the line needs to be split.
+ if (Columns > Style.ColumnLimit ||
+ (Annotations[i].MustBreakBefore &&
+ Annotations[i].Type != TokenAnnotation::TT_CtorInitializerColon)) {
+ FitsOnALine = false;
+ break;
+ }
+ }
+
// Start iterating at 1 as we have correctly formatted of Token #0 above.
for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
- unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
- unsigned Break = calcPenalty(State, true, NoBreak);
- addTokenToState(Break < NoBreak, false, State);
+ if (FitsOnALine) {
+ addTokenToState(false, false, State);
+ } else {
+ unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
+ unsigned Break = calcPenalty(State, true, NoBreak);
+ addTokenToState(Break < NoBreak, false, State);
+ }
}
}
@@ -146,9 +162,6 @@ private:
/// on a level.
std::vector<unsigned> FirstLessLess;
- bool CtorInitializerOnNewLine;
- bool InCtorInitializer;
-
/// \brief Comparison operator to be able to used \c IndentState in \c map.
bool operator<(const IndentState &Other) const {
if (Other.ConsumedTokens != ConsumedTokens)
@@ -212,11 +225,8 @@ private:
State.LastSpace[ParenLevel] = State.Indent[ParenLevel];
if (Current.Tok.is(tok::colon) &&
- Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr) {
+ Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr)
State.Indent[ParenLevel] += 2;
- State.CtorInitializerOnNewLine = true;
- State.InCtorInitializer = true;
- }
} else {
unsigned Spaces = Annotations[Index].SpaceRequiredBefore ? 1 : 0;
if (Annotations[Index].Type == TokenAnnotation::TT_LineComment)
@@ -228,10 +238,7 @@ private:
if (Previous.Tok.is(tok::l_paren) ||
Annotations[Index - 1].Type == TokenAnnotation::TT_TemplateOpener)
State.Indent[ParenLevel] = State.Column;
- if (Current.Tok.is(tok::colon)) {
- State.Indent[ParenLevel] = State.Column + 3;
- State.InCtorInitializer = true;
- }
+
// Top-level spaces are exempt as that mostly leads to better results.
State.Column += Spaces;
if (Spaces > 0 && ParenLevel != 0)
@@ -279,10 +286,8 @@ private:
"Tried to calculate penalty for splitting after the last token");
const FormatToken &Left = Line.Tokens[Index];
const FormatToken &Right = Line.Tokens[Index + 1];
- if (Left.Tok.is(tok::semi))
+ if (Left.Tok.is(tok::semi) || Left.Tok.is(tok::comma))
return 0;
- if (Left.Tok.is(tok::comma))
- return 1;
if (Left.Tok.is(tok::equal) || Left.Tok.is(tok::l_paren) ||
Left.Tok.is(tok::pipepipe) || Left.Tok.is(tok::ampamp))
return 2;
@@ -313,18 +318,10 @@ private:
if (NewLine && !Annotations[State.ConsumedTokens].CanBreakBefore)
return UINT_MAX;
- if (State.ConsumedTokens > 0 && !NewLine &&
- State.CtorInitializerOnNewLine &&
- Line.Tokens[State.ConsumedTokens - 1].Tok.is(tok::comma))
- return UINT_MAX;
-
- if (NewLine && State.InCtorInitializer && !State.CtorInitializerOnNewLine)
- return UINT_MAX;
-
unsigned CurrentPenalty = 0;
if (NewLine) {
CurrentPenalty += Parameters.PenaltyIndentLevel * State.Indent.size() +
- Parameters.PenaltyExtraLine + splitPenalty(State.ConsumedTokens - 1);
+ splitPenalty(State.ConsumedTokens - 1);
}
addTokenToState(NewLine, true, State);
@@ -413,9 +410,7 @@ class TokenAnnotator {
public:
TokenAnnotator(const UnwrappedLine &Line, const FormatStyle &Style,
SourceManager &SourceMgr)
- : Line(Line),
- Style(Style),
- SourceMgr(SourceMgr) {
+ : Line(Line), Style(Style), SourceMgr(SourceMgr) {
}
/// \brief A parser that gathers additional information about tokens.
@@ -427,9 +422,7 @@ public:
public:
AnnotatingParser(const SmallVector<FormatToken, 16> &Tokens,
std::vector<TokenAnnotation> &Annotations)
- : Tokens(Tokens),
- Annotations(Annotations),
- Index(0) {
+ : Tokens(Tokens), Annotations(Annotations), Index(0) {
}
bool parseAngle() {
@@ -496,6 +489,10 @@ public:
switch (Tokens[CurrentIndex].Tok.getKind()) {
case tok::l_paren:
parseParens();
+ if (Index < Tokens.size() && Tokens[Index].Tok.is(tok::colon)) {
+ Annotations[Index].Type = TokenAnnotation::TT_CtorInitializerColon;
+ next();
+ }
break;
case tok::l_square:
parseSquare();
@@ -557,7 +554,10 @@ public:
Annotation.CanBreakBefore =
canBreakBetween(Line.Tokens[i - 1], Line.Tokens[i]);
- if (Line.Tokens[i].Tok.is(tok::colon)) {
+ if (Annotation.Type == TokenAnnotation::TT_CtorInitializerColon) {
+ Annotation.MustBreakBefore = true;
+ Annotation.SpaceRequiredBefore = true;
+ } else if (Line.Tokens[i].Tok.is(tok::colon)) {
Annotation.SpaceRequiredBefore =
Line.Tokens[0].Tok.isNot(tok::kw_case) && i != e - 1;
} else if (Annotations[i - 1].Type == TokenAnnotation::TT_UnaryOperator) {
@@ -769,9 +769,7 @@ private:
class LexerBasedFormatTokenSource : public FormatTokenSource {
public:
LexerBasedFormatTokenSource(Lexer &Lex, SourceManager &SourceMgr)
- : GreaterStashed(false),
- Lex(Lex),
- SourceMgr(SourceMgr),
+ : GreaterStashed(false), Lex(Lex), SourceMgr(SourceMgr),
IdentTable(Lex.getLangOpts()) {
Lex.SetKeepWhitespaceMode(true);
}
@@ -831,10 +829,7 @@ class Formatter : public UnwrappedLineConsumer {
public:
Formatter(const FormatStyle &Style, Lexer &Lex, SourceManager &SourceMgr,
const std::vector<CharSourceRange> &Ranges)
- : Style(Style),
- Lex(Lex),
- SourceMgr(SourceMgr),
- Ranges(Ranges),
+ : Style(Style), Lex(Lex), SourceMgr(SourceMgr), Ranges(Ranges),
StructuralError(false) {
}
diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index fef56caee73..c9cd825277c 100644
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -374,6 +374,41 @@ TEST_F(FormatTest, FormatsAwesomeMethodCall) {
" parameter, parameter, parameter)), SecondLongCall(parameter));");
}
+TEST_F(FormatTest, ConstructorInitializers) {
+ verifyFormat("Constructor() : Initializer(FitsOnTheLine) {\n}");
+
+ verifyFormat(
+ "SomeClass::Constructor()\n"
+ " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"
+ "}");
+
+ verifyFormat(
+ "SomeClass::Constructor()\n"
+ " : aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
+ " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"
+ "}");
+
+ verifyFormat("Constructor()\n"
+ " : aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
+ " aaaaaaaaaaaaaaaaaaaaaaa() {\n"
+ "}");
+
+ // Here a line could be saved by splitting the second initializer onto two
+ // lines, but that is not desireable.
+ verifyFormat("Constructor()\n"
+ " : aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaa),\n"
+ " aaaaaaaaaaa(aaaaaaaaaaa),\n"
+ " aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
+ "}");
+
+ verifyGoogleFormat("MyClass::MyClass(int var)\n"
+ " : some_var_(var), // 4 space indent\n"
+ " some_other_var_(var + 1) { // lined up\n"
+ "}");
+}
+
TEST_F(FormatTest, BreaksAsHighAsPossible) {
verifyFormat(
"if ((aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa && aaaaaaaaaaaaaaaaaaaaaaaaaa) ||\n"
@@ -461,13 +496,11 @@ TEST_F(FormatTest, UnderstandsEquals) {
}
TEST_F(FormatTest, WrapsAtFunctionCallsIfNecessary) {
- verifyFormat(
- "LoooooooooooooooooooooooooooooooooooooongObject\n"
- " .looooooooooooooooooooooooooooooooooooooongFunction();");
+ verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
+ " .looooooooooooooooooooooooooooooooooooooongFunction();");
- verifyFormat(
- "LoooooooooooooooooooooooooooooooooooooongObject\n"
- " ->looooooooooooooooooooooooooooooooooooooongFunction();");
+ verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
+ " ->looooooooooooooooooooooooooooooooooooooongFunction();");
verifyFormat(
"LooooooooooooooooooooooooooooooooongObject->shortFunction(Parameter1,\n"
@@ -485,10 +518,9 @@ TEST_F(FormatTest, WrapsAtFunctionCallsIfNecessary) {
"function(LoooooooooooooooooooooooooooooooooooongObject\n"
" ->loooooooooooooooooooooooooooooooooooooooongFunction());");
- verifyFormat(
- "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
- " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
- "}");
+ verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
+ " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
+ "}");
}
TEST_F(FormatTest, UnderstandsTemplateParameters) {
--
GitLab