r192168 - clang-format: Improve constructor initializer linewrapping.
Manuel Klimek
klimek at google.com
Tue Oct 8 07:46:15 PDT 2013
On Tue, Oct 8, 2013 at 5:49 AM, Daniel Jasper <djasper at google.com> wrote:
>
>
>
> On Tue, Oct 8, 2013 at 2:20 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Mon, Oct 7, 2013 at 10:11 PM, Daniel Jasper <djasper at google.com>wrote:
>>
>>> Author: djasper
>>> Date: Tue Oct 8 00:11:18 2013
>>> New Revision: 192168
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=192168&view=rev
>>> Log:
>>> clang-format: Improve constructor initializer linewrapping.
>>>
>>> Specifically make ConstructorInitializerAllOnOneLineOrOnePerLine work
>>> nicely with BreakConstructorInitializersBeforeComma.
>>>
>>> This fixes llvm.org/PR17395.
>>>
>>> Modified:
>>> cfe/trunk/lib/Format/ContinuationIndenter.cpp
>>> cfe/trunk/lib/Format/TokenAnnotator.cpp
>>> cfe/trunk/unittests/Format/FormatTest.cpp
>>>
>>> Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=192168&r1=192167&r2=192168&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
>>> +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Tue Oct 8 00:11:18
>>> 2013
>>> @@ -44,6 +44,18 @@ static bool startsSegmentOfBuilderTypeCa
>>> return Tok.isMemberAccess() && Tok.Previous &&
>>> Tok.Previous->closesScope();
>>> }
>>>
>>> +// Returns \c true if \c Current starts a new parameter.
>>>
>>
I'd add:
// If true, 'Current' is the first token of either the next constructor
initializer, or the next
// parameter of any inner comma separated list.
// Note that if Style.BreakConstructorInitializersBeforeComma, inner
parameter lists
// will still start after the comma.
> +static bool startsNextParameter(const FormatToken &Current,
>>> + const FormatStyle &Style) {
>>> + const FormatToken &Previous = *Current.Previous;
>>> + if (Current.Type == TT_CtorInitializerComma &&
>>> + Style.BreakConstructorInitializersBeforeComma)
>>> + return true;
>>> + return Previous.is(tok::comma) && !Current.isTrailingComment() &&
>>> + (Previous.Type != TT_CtorInitializerComma ||
>>> + !Style.BreakConstructorInitializersBeforeComma);
>>>
>>
>> Isn't that last part redundant, given the first 'if'?
>>
>
> No. For one thing, Previous != Current.
>
Ah. I missed that. I wonder whether there's a better way to write that
down, perhaps in a way that the expression doesn't iterate between
previous->current->previous.
Here's a proposal that makes it easier to understand for me, to take or
leave ;)
if (Style.BreakConstructorInitializersBeforeComma) {
return Current.Type == TT_CtorInitializerComma ||
(Previous.Type != TT_CtorInitializerComma &&
Previous.is(tok::comma) && !Current.isTrailingComment());
} else {
return Previous.is(tok::comma) && !Current.isTrailingComment();
}
>
> +}
>>> +
>>> ContinuationIndenter::ContinuationIndenter(const FormatStyle &Style,
>>> SourceManager &SourceMgr,
>>> WhitespaceManager
>>> &Whitespaces,
>>> @@ -113,15 +125,9 @@ bool ContinuationIndenter::mustBreak(con
>>> return true;
>>> if (Previous.is(tok::semi) &&
>>> State.LineContainsContinuedForLoopSection)
>>> return true;
>>> - if (Style.BreakConstructorInitializersBeforeComma) {
>>> - if (Previous.Type == TT_CtorInitializerComma)
>>> - return false;
>>> - if (Current.Type == TT_CtorInitializerComma)
>>> - return true;
>>> - }
>>> - if ((Previous.isOneOf(tok::comma, tok::semi) ||
>>> Current.is(tok::question) ||
>>> - (Current.Type == TT_ConditionalExpr &&
>>> - !(Current.is(tok::colon) && Previous.is(tok::question)))) &&
>>> + if ((startsNextParameter(Current, Style) || Previous.is(tok::semi) ||
>>> + Current.is(tok::question) ||
>>> + (Current.Type == TT_ConditionalExpr &&
>>> Previous.isNot(tok::question))) &&
>>> State.Stack.back().BreakBeforeParameter &&
>>> !Current.isTrailingComment() &&
>>> !Current.isOneOf(tok::r_paren, tok::r_brace))
>>> return true;
>>> @@ -250,8 +256,7 @@ void ContinuationIndenter::addTokenOnCur
>>> if (Previous.opensScope() && Previous.Type != TT_ObjCMethodExpr &&
>>> Current.Type != TT_LineComment)
>>> State.Stack.back().Indent = State.Column + Spaces;
>>> - if (Previous.is(tok::comma) && !Current.isTrailingComment() &&
>>> - State.Stack.back().AvoidBinPacking)
>>> + if (State.Stack.back().AvoidBinPacking &&
>>> startsNextParameter(Current, Style))
>>> State.Stack.back().NoLineBreak = true;
>>> if (startsSegmentOfBuilderTypeCall(Current))
>>> State.Stack.back().ContainsUnwrappedBuilder = true;
>>>
>>> Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=192168&r1=192167&r2=192168&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
>>> +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Tue Oct 8 00:11:18 2013
>>> @@ -1366,7 +1366,8 @@ bool TokenAnnotator::mustBreakBefore(con
>>> // FIXME: Fix horrible hack of using BindingStrength to find
>>> top-level <>.
>>> return true;
>>> } else if (Right.Type == TT_CtorInitializerComma &&
>>> - Style.BreakConstructorInitializersBeforeComma) {
>>> + Style.BreakConstructorInitializersBeforeComma &&
>>> + !Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
>>> return true;
>>> } else if (Right.Previous->BlockKind == BK_Block &&
>>> Right.Previous->isNot(tok::r_brace) &&
>>> @@ -1450,6 +1451,9 @@ bool TokenAnnotator::canBreakBefore(cons
>>> if (Left.Type == TT_CtorInitializerComma &&
>>> Style.BreakConstructorInitializersBeforeComma)
>>> return false;
>>> + if (Right.Type == TT_CtorInitializerComma &&
>>> + Style.BreakConstructorInitializersBeforeComma)
>>> + return true;
>>> if (Right.isBinaryOperator() && Style.BreakBeforeBinaryOperators)
>>> return true;
>>> if (Left.is(tok::greater) && Right.is(tok::greater) &&
>>>
>>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=192168&r1=192167&r2=192168&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Oct 8 00:11:18 2013
>>> @@ -6587,6 +6587,20 @@ TEST_F(FormatTest, ConstructorInitialize
>>> ", b(b)\n"
>>> ", c(c) {}",
>>> Style);
>>> +
>>> + Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
>>> + Style.ConstructorInitializerIndentWidth = 4;
>>> + verifyFormat(
>>> + "SomeClass::Constructor()\n"
>>> + " : aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaa)
>>> {}",
>>> + Style);
>>> + Style.ConstructorInitializerIndentWidth = 4;
>>> + Style.ColumnLimit = 60;
>>> + verifyFormat("SomeClass::Constructor()\n"
>>> + " : aaaaaaaa(aaaaaaaa)\n"
>>> + " , aaaaaaaa(aaaaaaaa)\n"
>>> + " , aaaaaaaa(aaaaaaaa) {}",
>>> + Style);
>>> }
>>>
>>> TEST_F(FormatTest, FormatsWithWebKitStyle) {
>>>
>>>
>>> _______________________________________________
>>> 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/20131008/2eb4dd1d/attachment.html>
More information about the cfe-commits
mailing list