<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">Also, I entirely removed the penalty for the extra line count as it turns out that the actual line count is not really correlated with readability. Also, due to the split penalty in the different cases, clang-format already keeps the line count low.<div>
<br></div><div>This was necessary to fix the test case with the corresponding comment.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 18, 2012 at 10:05 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: djasper<br>
Date: Tue Dec 18 15:05:13 2012<br>
New Revision: 170457<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=170457&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=170457&view=rev</a><br>
Log:<br>
Better support for constructor initializers.<br>
<br>
We used to format initializers like this (with a sort of hacky implementation):<br>
Constructor()<br>
: Val1(A),<br>
Val2(B) {<br>
<br>
and now format like this (with a somewhat better solution):<br>
Constructor()<br>
: Val1(A), Val2(B) {<br>
<br>
assuming this would not fit on a single line. Also added tests.<br>
<br>
As a side effect we now first analyze whether an UnwrappedLine needs to be<br>
split at all. If not, not splitting it is the best solution by definition. As<br>
this should be a very common case in normal code, not exploring the entire<br>
solution space can provide significant speedup.<br>
<br>
Modified:<br>
cfe/trunk/lib/Format/Format.cpp<br>
cfe/trunk/unittests/Format/FormatTest.cpp<br>
<br>
Modified: cfe/trunk/lib/Format/Format.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=170457&r1=170456&r2=170457&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=170457&r1=170456&r2=170457&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/Format.cpp (original)<br>
+++ cfe/trunk/lib/Format/Format.cpp Tue Dec 18 15:05:13 2012<br>
@@ -37,6 +37,7 @@<br>
TT_OverloadedOperator,<br>
TT_PointerOrReference,<br>
TT_ConditionalExpr,<br>
+ TT_CtorInitializerColon,<br>
TT_LineComment,<br>
TT_BlockComment<br>
};<br>
@@ -73,7 +74,6 @@<br>
}<br>
<br>
struct OptimizationParameters {<br>
- unsigned PenaltyExtraLine;<br>
unsigned PenaltyIndentLevel;<br>
};<br>
<br>
@@ -83,13 +83,9 @@<br>
const UnwrappedLine &Line,<br>
const std::vector<TokenAnnotation> &Annotations,<br>
tooling::Replacements &Replaces, bool StructuralError)<br>
- : Style(Style),<br>
- SourceMgr(SourceMgr),<br>
- Line(Line),<br>
- Annotations(Annotations),<br>
- Replaces(Replaces),<br>
+ : Style(Style), SourceMgr(SourceMgr), Line(Line),<br>
+ Annotations(Annotations), Replaces(Replaces),<br>
StructuralError(StructuralError) {<br>
- Parameters.PenaltyExtraLine = 100;<br>
Parameters.PenaltyIndentLevel = 5;<br>
}<br>
<br>
@@ -100,8 +96,6 @@<br>
// Initialize state dependent on indent.<br>
IndentState State;<br>
State.Column = Indent;<br>
- State.CtorInitializerOnNewLine = false;<br>
- State.InCtorInitializer = false;<br>
State.ConsumedTokens = 0;<br>
State.Indent.push_back(Indent + 4);<br>
State.LastSpace.push_back(Indent);<br>
@@ -110,11 +104,33 @@<br>
// The first token has already been indented and thus consumed.<br>
moveStateToNextToken(State);<br>
<br>
+ // Check whether the UnwrappedLine can be put onto a single line. If so,<br>
+ // this is bound to be the optimal solution (by definition) and we don't<br>
+ // need to analyze the entire solution space.<br>
+ unsigned Columns = State.Column;<br>
+ bool FitsOnALine = true;<br>
+ for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {<br>
+ Columns += (Annotations[i].SpaceRequiredBefore ? 1 : 0) +<br>
+ Line.Tokens[i].Tok.getLength();<br>
+ // A special case for the colon of a constructor initializer as this only<br>
+ // needs to be put on a new line if the line needs to be split.<br>
+ if (Columns > Style.ColumnLimit ||<br>
+ (Annotations[i].MustBreakBefore &&<br>
+ Annotations[i].Type != TokenAnnotation::TT_CtorInitializerColon)) {<br>
+ FitsOnALine = false;<br>
+ break;<br>
+ }<br>
+ }<br>
+<br>
// Start iterating at 1 as we have correctly formatted of Token #0 above.<br>
for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {<br>
- unsigned NoBreak = calcPenalty(State, false, UINT_MAX);<br>
- unsigned Break = calcPenalty(State, true, NoBreak);<br>
- addTokenToState(Break < NoBreak, false, State);<br>
+ if (FitsOnALine) {<br>
+ addTokenToState(false, false, State);<br>
+ } else {<br>
+ unsigned NoBreak = calcPenalty(State, false, UINT_MAX);<br>
+ unsigned Break = calcPenalty(State, true, NoBreak);<br>
+ addTokenToState(Break < NoBreak, false, State);<br>
+ }<br>
}<br>
}<br>
<br>
@@ -146,9 +162,6 @@<br>
/// on a level.<br>
std::vector<unsigned> FirstLessLess;<br>
<br>
- bool CtorInitializerOnNewLine;<br>
- bool InCtorInitializer;<br>
-<br>
/// \brief Comparison operator to be able to used \c IndentState in \c map.<br>
bool operator<(const IndentState &Other) const {<br>
if (Other.ConsumedTokens != ConsumedTokens)<br>
@@ -212,11 +225,8 @@<br>
<br>
State.LastSpace[ParenLevel] = State.Indent[ParenLevel];<br>
if (<a href="http://Current.Tok.is" target="_blank">Current.Tok.is</a>(tok::colon) &&<br>
- Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr) {<br>
+ Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr)<br>
State.Indent[ParenLevel] += 2;<br>
- State.CtorInitializerOnNewLine = true;<br>
- State.InCtorInitializer = true;<br>
- }<br>
} else {<br>
unsigned Spaces = Annotations[Index].SpaceRequiredBefore ? 1 : 0;<br>
if (Annotations[Index].Type == TokenAnnotation::TT_LineComment)<br>
@@ -228,10 +238,7 @@<br>
if (<a href="http://Previous.Tok.is" target="_blank">Previous.Tok.is</a>(tok::l_paren) ||<br>
Annotations[Index - 1].Type == TokenAnnotation::TT_TemplateOpener)<br>
State.Indent[ParenLevel] = State.Column;<br>
- if (<a href="http://Current.Tok.is" target="_blank">Current.Tok.is</a>(tok::colon)) {<br>
- State.Indent[ParenLevel] = State.Column + 3;<br>
- State.InCtorInitializer = true;<br>
- }<br>
+<br>
// Top-level spaces are exempt as that mostly leads to better results.<br>
State.Column += Spaces;<br>
if (Spaces > 0 && ParenLevel != 0)<br>
@@ -279,10 +286,8 @@<br>
"Tried to calculate penalty for splitting after the last token");<br>
const FormatToken &Left = Line.Tokens[Index];<br>
const FormatToken &Right = Line.Tokens[Index + 1];<br>
- if (<a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::semi))<br>
+ if (<a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::semi) || <a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::comma))<br>
return 0;<br>
- if (<a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::comma))<br>
- return 1;<br>
if (<a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::equal) || <a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::l_paren) ||<br>
<a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::pipepipe) || <a href="http://Left.Tok.is" target="_blank">Left.Tok.is</a>(tok::ampamp))<br>
return 2;<br>
@@ -313,18 +318,10 @@<br>
if (NewLine && !Annotations[State.ConsumedTokens].CanBreakBefore)<br>
return UINT_MAX;<br>
<br>
- if (State.ConsumedTokens > 0 && !NewLine &&<br>
- State.CtorInitializerOnNewLine &&<br>
- Line.Tokens[State.ConsumedTokens - 1].Tok.is(tok::comma))<br>
- return UINT_MAX;<br>
-<br>
- if (NewLine && State.InCtorInitializer && !State.CtorInitializerOnNewLine)<br>
- return UINT_MAX;<br>
-<br>
unsigned CurrentPenalty = 0;<br>
if (NewLine) {<br>
CurrentPenalty += Parameters.PenaltyIndentLevel * State.Indent.size() +<br>
- Parameters.PenaltyExtraLine + splitPenalty(State.ConsumedTokens - 1);<br>
+ splitPenalty(State.ConsumedTokens - 1);<br>
}<br>
<br>
addTokenToState(NewLine, true, State);<br>
@@ -413,9 +410,7 @@<br>
public:<br>
TokenAnnotator(const UnwrappedLine &Line, const FormatStyle &Style,<br>
SourceManager &SourceMgr)<br>
- : Line(Line),<br>
- Style(Style),<br>
- SourceMgr(SourceMgr) {<br>
+ : Line(Line), Style(Style), SourceMgr(SourceMgr) {<br>
}<br>
<br>
/// \brief A parser that gathers additional information about tokens.<br>
@@ -427,9 +422,7 @@<br>
public:<br>
AnnotatingParser(const SmallVector<FormatToken, 16> &Tokens,<br>
std::vector<TokenAnnotation> &Annotations)<br>
- : Tokens(Tokens),<br>
- Annotations(Annotations),<br>
- Index(0) {<br>
+ : Tokens(Tokens), Annotations(Annotations), Index(0) {<br>
}<br>
<br>
bool parseAngle() {<br>
@@ -496,6 +489,10 @@<br>
switch (Tokens[CurrentIndex].Tok.getKind()) {<br>
case tok::l_paren:<br>
parseParens();<br>
+ if (Index < Tokens.size() && Tokens[Index].Tok.is(tok::colon)) {<br>
+ Annotations[Index].Type = TokenAnnotation::TT_CtorInitializerColon;<br>
+ next();<br>
+ }<br>
break;<br>
case tok::l_square:<br>
parseSquare();<br>
@@ -557,7 +554,10 @@<br>
Annotation.CanBreakBefore =<br>
canBreakBetween(Line.Tokens[i - 1], Line.Tokens[i]);<br>
<br>
- if (Line.Tokens[i].Tok.is(tok::colon)) {<br>
+ if (Annotation.Type == TokenAnnotation::TT_CtorInitializerColon) {<br>
+ Annotation.MustBreakBefore = true;<br>
+ Annotation.SpaceRequiredBefore = true;<br>
+ } else if (Line.Tokens[i].Tok.is(tok::colon)) {<br>
Annotation.SpaceRequiredBefore =<br>
Line.Tokens[0].Tok.isNot(tok::kw_case) && i != e - 1;<br>
} else if (Annotations[i - 1].Type == TokenAnnotation::TT_UnaryOperator) {<br>
@@ -769,9 +769,7 @@<br>
class LexerBasedFormatTokenSource : public FormatTokenSource {<br>
public:<br>
LexerBasedFormatTokenSource(Lexer &Lex, SourceManager &SourceMgr)<br>
- : GreaterStashed(false),<br>
- Lex(Lex),<br>
- SourceMgr(SourceMgr),<br>
+ : GreaterStashed(false), Lex(Lex), SourceMgr(SourceMgr),<br>
IdentTable(Lex.getLangOpts()) {<br>
Lex.SetKeepWhitespaceMode(true);<br>
}<br>
@@ -831,10 +829,7 @@<br>
public:<br>
Formatter(const FormatStyle &Style, Lexer &Lex, SourceManager &SourceMgr,<br>
const std::vector<CharSourceRange> &Ranges)<br>
- : Style(Style),<br>
- Lex(Lex),<br>
- SourceMgr(SourceMgr),<br>
- Ranges(Ranges),<br>
+ : Style(Style), Lex(Lex), SourceMgr(SourceMgr), Ranges(Ranges),<br>
StructuralError(false) {<br>
}<br>
<br>
<br>
Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=170457&r1=170456&r2=170457&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=170457&r1=170456&r2=170457&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Dec 18 15:05:13 2012<br>
@@ -374,6 +374,41 @@<br>
" parameter, parameter, parameter)), SecondLongCall(parameter));");<br>
}<br>
<br>
+TEST_F(FormatTest, ConstructorInitializers) {<br>
+ verifyFormat("Constructor() : Initializer(FitsOnTheLine) {\n}");<br>
+<br>
+ verifyFormat(<br>
+ "SomeClass::Constructor()\n"<br>
+ " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"<br>
+ "}");<br>
+<br>
+ verifyFormat(<br>
+ "SomeClass::Constructor()\n"<br>
+ " : aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"<br>
+ " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"<br>
+ "}");<br>
+<br>
+ verifyFormat("Constructor()\n"<br>
+ " : aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaa() {\n"<br>
+ "}");<br>
+<br>
+ // Here a line could be saved by splitting the second initializer onto two<br>
+ // lines, but that is not desireable.<br>
+ verifyFormat("Constructor()\n"<br>
+ " : aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaa),\n"<br>
+ " aaaaaaaaaaa(aaaaaaaaaaa),\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"<br>
+ "}");<br>
+<br>
+ verifyGoogleFormat("MyClass::MyClass(int var)\n"<br>
+ " : some_var_(var), // 4 space indent\n"<br>
+ " some_other_var_(var + 1) { // lined up\n"<br>
+ "}");<br>
+}<br>
+<br>
TEST_F(FormatTest, BreaksAsHighAsPossible) {<br>
verifyFormat(<br>
"if ((aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa && aaaaaaaaaaaaaaaaaaaaaaaaaa) ||\n"<br>
@@ -461,13 +496,11 @@<br>
}<br>
<br>
TEST_F(FormatTest, WrapsAtFunctionCallsIfNecessary) {<br>
- verifyFormat(<br>
- "LoooooooooooooooooooooooooooooooooooooongObject\n"<br>
- " .looooooooooooooooooooooooooooooooooooooongFunction();");<br>
+ verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"<br>
+ " .looooooooooooooooooooooooooooooooooooooongFunction();");<br>
<br>
- verifyFormat(<br>
- "LoooooooooooooooooooooooooooooooooooooongObject\n"<br>
- " ->looooooooooooooooooooooooooooooooooooooongFunction();");<br>
+ verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"<br>
+ " ->looooooooooooooooooooooooooooooooooooooongFunction();");<br>
<br>
verifyFormat(<br>
"LooooooooooooooooooooooooooooooooongObject->shortFunction(Parameter1,\n"<br>
@@ -485,10 +518,9 @@<br>
"function(LoooooooooooooooooooooooooooooooooooongObject\n"<br>
" ->loooooooooooooooooooooooooooooooooooooooongFunction());");<br>
<br>
- verifyFormat(<br>
- "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"<br>
- " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"<br>
- "}");<br>
+ verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"<br>
+ " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"<br>
+ "}");<br>
}<br>
<br>
TEST_F(FormatTest, UnderstandsTemplateParameters) {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>