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