<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">As for Chromium and WebKit coding styles, I am already looking at them specifically, as they will require some things that are much easier in Google/LLVM. As for configuration, the coding styles are currently hard-coded mostly because it is easiest, makes clang-format easy to use and avoids configuration files running out of sync with the clang-format version. Once we are a bit further along (maybe soon-ish), I agree that config files are a better solution at least as an additional option.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 18, 2012 at 10:45 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt">Out of interest, what's the plan for adding more coding styles? It looks like there's an coding style enum somewhere; are projects who don't use one of the existing coding styles expected to contribute patches to clang itself for their styles? Or will there be some config file eventually?<div>
<br></div><div>I'm asking because webkit style for initializers prefers one line per initializer (and leading commas): <a href="http://www.webkit.org/coding/coding-style.html#punctuation" target="_blank">http://www.webkit.org/coding/coding-style.html#punctuation</a></div>
<span class="HOEnZb"><font color="#888888">
<div><br></div></font></span><div><span class="HOEnZb"><font color="#888888">Nico<br><br></font></span><div class="gmail_quote"><div class="im">On Tue, Dec 18, 2012 at 1:05 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
</div><div><div class="h5"><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" target="_blank">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></div></div><br></div></div>
</blockquote></div><br></div></div>