<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Feb 24, 2013 at 11:15 AM, 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"><div dir="ltr">Right, but it is consistent with the splitting before a function name, which is exclusively done after & and * in LLVM/Clang.</div>
</blockquote><div><br></div><div style>Curious/fair point.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">On Sun, Feb 24, 2013 at 7:58 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Sun, Feb 24, 2013 at 10:54 AM, 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: Sun Feb 24 12:54:32 2013<br>
New Revision: 175999<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=175999&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=175999&view=rev</a><br>
Log:<br>
Allow breaking between a type and name in variable declarations.<br>
<br>
This fixes <a href="http://llvm.org/PR14967" target="_blank">llvm.org/PR14967</a> and is generall necessary to avoid<br>
situations where the column limit is exceeded. The challenge is<br>
restricting such lines splits, otherwise clang-format suddenly starts<br>
breaking at bad places.<br>
<br>
Before:<br>
ReallyLongReturnType<TemplateParam1, TemplateParam2><br>
ReallyReallyLongFunctionName(<br>
const std::string &SomeParameter,<br>
const SomeType<string,<br>
SomeOtherTemplateParameter> &ReallyReallyLongParameterName,<br>
const SomeType<string,<br>
SomeOtherTemplateParameter> &AnotherLongParameterName) {}<br>
<br>
After:<br>
ReallyLongReturnType<TemplateParam1, TemplateParam2><br>
ReallyReallyLongFunctionName(<br>
const std::string &SomeParameter,<br>
const SomeType<string, SomeOtherTemplateParameter> &<br>
ReallyReallyLongParameterName,<br>
const SomeType<string, SomeOtherTemplateParameter> &<br>
AnotherLongParameterName) {}<br></blockquote><div><br></div></div></div><div>The placement of the '&' seems to go against the spirit of "put it with the variable, not the type" style.</div><div>
<div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
cfe/trunk/lib/Format/Format.cpp<br>
cfe/trunk/lib/Format/TokenAnnotator.cpp<br>
cfe/trunk/lib/Format/TokenAnnotator.h<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=175999&r1=175998&r2=175999&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=175999&r1=175998&r2=175999&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/Format.cpp (original)<br>
+++ cfe/trunk/lib/Format/Format.cpp Sun Feb 24 12:54:32 2013<br>
@@ -258,12 +258,6 @@ private:<br>
tooling::Replacements Replaces;<br>
};<br>
<br>
-static bool isVarDeclName(const AnnotatedToken &Tok) {<br>
- return Tok.Parent != NULL && Tok.is(tok::identifier) &&<br>
- (Tok.Parent->Type == TT_PointerOrReference ||<br>
- Tok.Parent->is(tok::identifier));<br>
-}<br>
-<br>
class UnwrappedLineFormatter {<br>
public:<br>
UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr,<br>
@@ -498,8 +492,8 @@ private:<br>
((RootToken.is(tok::kw_for) && State.ParenLevel == 1) ||<br>
State.ParenLevel == 0)) {<br>
State.Column = State.VariablePos;<br>
- } else if (State.NextToken->Parent->ClosesTemplateDeclaration ||<br>
- Current.Type == TT_StartOfName) {<br>
+ } else if (Previous.ClosesTemplateDeclaration ||<br>
+ (Current.Type == TT_StartOfName && State.ParenLevel == 0)) {<br>
State.Column = State.Stack.back().Indent - 4;<br>
} else if (Current.Type == TT_ObjCSelectorName) {<br>
if (State.Stack.back().ColonPos > Current.FormatTok.TokenLength) {<br>
@@ -510,7 +504,8 @@ private:<br>
State.Stack.back().ColonPos =<br>
State.Column + Current.FormatTok.TokenLength;<br>
}<br>
- } else if (Previous.Type == TT_ObjCMethodExpr || isVarDeclName(Current)) {<br>
+ } else if (Previous.Type == TT_ObjCMethodExpr ||<br>
+ Current.Type == TT_StartOfName) {<br>
State.Column = State.Stack.back().Indent + 4;<br>
} else {<br>
State.Column = State.Stack.back().Indent;<br>
@@ -551,8 +546,7 @@ private:<br>
if (State.Stack.back().AvoidBinPacking) {<br>
// If we are breaking after '(', '{', '<', this is not bin packing<br>
// unless AllowAllParametersOfDeclarationOnNextLine is false.<br>
- if ((Previous.isNot(tok::l_paren) && Previous.isNot(tok::l_brace) &&<br>
- Previous.Type != TT_TemplateOpener) ||<br>
+ if ((Previous.isNot(tok::l_paren) && Previous.isNot(tok::l_brace)) ||<br>
(!Style.AllowAllParametersOfDeclarationOnNextLine &&<br>
Line.MustBeDeclaration))<br>
State.Stack.back().BreakBeforeParameter = true;<br>
<br>
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=175999&r1=175998&r2=175999&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=175999&r1=175998&r2=175999&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)<br>
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Sun Feb 24 12:54:32 2013<br>
@@ -83,7 +83,6 @@ public:<br>
: SourceMgr(SourceMgr), Lex(Lex), Line(Line), CurrentToken(&Line.First),<br>
KeywordVirtualFound(false), Ident_in(Ident_in) {<br>
Contexts.push_back(Context(1, /*IsExpression=*/ false));<br>
- Contexts.back().LookForFunctionName = Line.MustBeDeclaration;<br>
}<br>
<br>
private:<br>
@@ -348,6 +347,8 @@ private:<br>
case tok::l_paren:<br>
if (!parseParens())<br>
return false;<br>
+ if (Line.MustBeDeclaration)<br>
+ Line.MightBeFunctionDecl = true;<br>
break;<br>
case tok::l_square:<br>
if (!parseSquare())<br>
@@ -520,8 +521,7 @@ private:<br>
Context(unsigned BindingStrength, bool IsExpression)<br>
: BindingStrength(BindingStrength), LongestObjCSelectorName(0),<br>
ColonIsForRangeExpr(false), ColonIsObjCMethodExpr(false),<br>
- FirstObjCSelectorName(NULL), IsExpression(IsExpression),<br>
- LookForFunctionName(false) {}<br>
+ FirstObjCSelectorName(NULL), IsExpression(IsExpression) {}<br>
<br>
unsigned BindingStrength;<br>
unsigned LongestObjCSelectorName;<br>
@@ -529,7 +529,6 @@ private:<br>
bool ColonIsObjCMethodExpr;<br>
AnnotatedToken *FirstObjCSelectorName;<br>
bool IsExpression;<br>
- bool LookForFunctionName;<br>
};<br>
<br>
/// \brief Puts a new \c Context onto the stack \c Contexts for the lifetime<br>
@@ -572,9 +571,13 @@ private:<br>
}<br>
<br>
if (Current.Type == TT_Unknown) {<br>
- if (Contexts.back().LookForFunctionName && Current.is(tok::l_paren)) {<br>
- findFunctionName(&Current);<br>
- Contexts.back().LookForFunctionName = false;<br>
+ if (Current.Parent && Current.is(tok::identifier) &&<br>
+ ((Current.Parent->is(tok::identifier) &&<br>
+ Current.Parent->FormatTok.Tok.getIdentifierInfo()<br>
+ ->getPPKeywordID() == tok::pp_not_keyword) ||<br>
+ Current.Parent->Type == TT_PointerOrReference ||<br>
+ Current.Parent->Type == TT_TemplateCloser)) {<br>
+ Current.Type = TT_StartOfName;<br>
} else if (Current.is(tok::star) || Current.is(tok::amp)) {<br>
Current.Type =<br>
determineStarAmpUsage(Current, Contexts.back().IsExpression);<br>
@@ -623,22 +626,6 @@ private:<br>
}<br>
}<br>
<br>
- /// \brief Starting from \p Current, this searches backwards for an<br>
- /// identifier which could be the start of a function name and marks it.<br>
- void findFunctionName(AnnotatedToken *Current) {<br>
- AnnotatedToken *Parent = Current->Parent;<br>
- while (Parent != NULL && Parent->Parent != NULL) {<br>
- if (Parent->is(tok::identifier) &&<br>
- (Parent->Parent->is(tok::identifier) ||<br>
- Parent->Parent->Type == TT_PointerOrReference ||<br>
- Parent->Parent->Type == TT_TemplateCloser)) {<br>
- Parent->Type = TT_StartOfName;<br>
- break;<br>
- }<br>
- Parent = Parent->Parent;<br>
- }<br>
- }<br>
-<br>
/// \brief Return the type of the given token assuming it is * or &.<br>
TokenType<br>
determineStarAmpUsage(const AnnotatedToken &Tok, bool IsExpression) {<br>
@@ -883,8 +870,15 @@ unsigned TokenAnnotator::splitPenalty(co<br>
const AnnotatedToken &Left = *Tok.Parent;<br>
const AnnotatedToken &Right = Tok;<br>
<br>
- if (Right.Type == TT_StartOfName)<br>
- return Style.PenaltyReturnTypeOnItsOwnLine;<br>
+ if (Right.Type == TT_StartOfName) {<br>
+ if (<a href="http://Line.First.is" target="_blank">Line.First.is</a>(tok::kw_for))<br>
+ return 3;<br>
+ else if (Line.MightBeFunctionDecl && Right.BindingStrength == 1)<br>
+ // FIXME: Clean up hack of using BindingStrength to find top-level names.<br>
+ return Style.PenaltyReturnTypeOnItsOwnLine;<br>
+ else<br>
+ return 100;<br>
+ }<br>
if (Left.is(tok::l_brace) && Right.isNot(tok::l_brace))<br>
return 50;<br>
if (Left.is(tok::equal) && Right.is(tok::l_brace))<br>
@@ -941,7 +935,7 @@ unsigned TokenAnnotator::splitPenalty(co<br>
<br>
if (Level != prec::Unknown)<br>
return Level;<br>
-<br>
+<br>
return 3;<br>
}<br>
<br>
@@ -1099,12 +1093,6 @@ bool TokenAnnotator::canBreakBefore(cons<br>
// change the "binding" behavior of a comment.<br>
return false;<br>
<br>
- // FIXME: We can probably remove this special case once we have implemented<br>
- // breaking after types in general.<br>
- if (<a href="http://Line.First.is" target="_blank">Line.First.is</a>(tok::kw_for) && !Right.Children.empty() &&<br>
- Right.Children[0].is(tok::equal))<br>
- return true;<br>
-<br>
// Allow breaking after a trailing 'const', e.g. after a method declaration,<br>
// unless it is follow by ';', '{' or '='.<br>
if (Left.is(tok::kw_const) && Left.Parent != NULL &&<br>
<br>
Modified: cfe/trunk/lib/Format/TokenAnnotator.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.h?rev=175999&r1=175998&r2=175999&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.h?rev=175999&r1=175998&r2=175999&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/TokenAnnotator.h (original)<br>
+++ cfe/trunk/lib/Format/TokenAnnotator.h Sun Feb 24 12:54:32 2013<br>
@@ -140,7 +140,8 @@ public:<br>
AnnotatedLine(const UnwrappedLine &Line)<br>
: First(Line.Tokens.front()), Level(Line.Level),<br>
InPPDirective(Line.InPPDirective),<br>
- MustBeDeclaration(Line.MustBeDeclaration) {<br>
+ MustBeDeclaration(Line.MustBeDeclaration),<br>
+ MightBeFunctionDecl(false) {<br>
assert(!Line.Tokens.empty());<br>
AnnotatedToken *Current = &First;<br>
for (std::list<FormatToken>::const_iterator I = ++Line.Tokens.begin(),<br>
@@ -155,7 +156,8 @@ public:<br>
AnnotatedLine(const AnnotatedLine &Other)<br>
: First(Other.First), Type(Other.Type), Level(Other.Level),<br>
InPPDirective(Other.InPPDirective),<br>
- MustBeDeclaration(Other.MustBeDeclaration) {<br>
+ MustBeDeclaration(Other.MustBeDeclaration),<br>
+ MightBeFunctionDecl(Other.MightBeFunctionDecl) {<br>
Last = &First;<br>
while (!Last->Children.empty()) {<br>
Last->Children[0].Parent = Last;<br>
@@ -170,6 +172,7 @@ public:<br>
unsigned Level;<br>
bool InPPDirective;<br>
bool MustBeDeclaration;<br>
+ bool MightBeFunctionDecl;<br>
};<br>
<br>
inline prec::Level getPrecedence(const AnnotatedToken &Tok) {<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=175999&r1=175998&r2=175999&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175999&r1=175998&r2=175999&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Sun Feb 24 12:54:32 2013<br>
@@ -1931,13 +1931,24 @@ TEST_F(FormatTest, FormatsFunctionTypes)<br>
verifyFormat("int(*func)(void *);");<br>
}<br>
<br>
-TEST_F(FormatTest, BreaksFunctionDeclarations) {<br>
+TEST_F(FormatTest, BreaksLongDeclarations) {<br>
verifyFormat("int *someFunction(int LoooooooooooooooooooongParam1,\n"<br>
" int LoooooooooooooooooooongParam2) {}");<br>
verifyFormat(<br>
"TypeSpecDecl *\n"<br>
"TypeSpecDecl::Create(ASTContext &C, DeclContext *DC, SourceLocation L,\n"<br>
" IdentifierIn *II, Type *T) {}");<br>
+ verifyFormat("ReallyLongReturnType<TemplateParam1, TemplateParam2>\n"<br>
+ "ReallyReallyLongFunctionName(\n"<br>
+ " const std::string &SomeParameter,\n"<br>
+ " const SomeType<string, SomeOtherTemplateParameter> &\n"<br>
+ " ReallyReallyLongParameterName,\n"<br>
+ " const SomeType<string, SomeOtherTemplateParameter> &\n"<br>
+ " AnotherLongParameterName) {}");<br>
+ verifyFormat(<br>
+ "aaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaa<aaaaaaaaaaaaa, aaaaaaaaaaaa>\n"<br>
+ "aaaaaaaaaaaaaaaaaaaaaaa;");<br>
+<br>
verifyGoogleFormat(<br>
"TypeSpecDecl* TypeSpecDecl::Create(\n"<br>
" ASTContext& C, DeclContext* DC, SourceLocation L) {}");<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></div></blockquote></div><br></div></div>