russellmcc added inline comments.
================
Comment at: lib/Format/ContinuationIndenter.cpp:760
(!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration) ||
+ (!Style.AllowAllArgumentsOnNextLine &&
----------------
djasper wrote:
> This still looks suspicious to me. State.Line->MustBeDeclaration is either
> true or false meaning that AllowAllParametersOfDeclarationOnNextLine or
> AllowAllArgumentsOnNextLine can always affect the behavior here, leading to
> BreakBeforeParameter to be set to true, even if we are in the case for
> PreviousIsBreakingCtorInitializerColon being true.
>
> So, my guess would be that if you set one of AllowAllArgumentsOnNextLine and
> AllowAllParametersOfDeclarationOnNextLine to false, then
> AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore.
>
> Please verify, and if this is true, please fix and add tests. I think this
> might be easier to understand if you pulled the one if statement apart.
Actually, I think this logic works. The case you are worried about
(interaction between arguments, parameters, and constructor initializers) is
already tested in the unit tests in the
`AllowAllConstructorInitializersOnNextLine` test. The specific concern you
have is solved by the separate if statement below.
I agree that this logic is a bit complex, but I think it's necessary since in
most cases we don't want to change the existing value of
`State.Stack.back().BreakBeforeParameter` - we only want to change this when we
are sure we should or shouldn't bin-pack. I've tried hard not to change any
existing behavior unless it was clearly a bug. I think we could simplify this
logic if we wanted to be less conservative.
I'm not sure what you mean by breaking up the if statement - did you mean
something like this? To me, this reads much more verbose and is a bit more
confusing; however I'm happy to edit the diff if it makes more sense to you:
```
// If we are breaking after '(', '{', '<', or this is the break after a ':'
// to start a member initializater list in a constructor, this should not
// be considered bin packing unless the relevant AllowAll option is false or
// this is a dict/object literal.
bool PreviousIsBreakingCtorInitializerColon =
Previous.is(TT_CtorInitializerColon) &&
Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
if (!(Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
PreviousIsBreakingCtorInitializerColon))
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration)
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration)
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllConstructorInitializersOnNextLine &&
PreviousIsBreakingCtorInitializerColon)
State.Stack.back().BreakBeforeParameter = true;
if (Previous.is(TT_DictLiteral)))
State.Stack.back().BreakBeforeParameter = true;
// If we are breaking after a ':' to start a member initializer list,
// and we allow all arguments on the next line, we should not break
// before the next parameter.
if (PreviousIsBreakingCtorInitializerColon &&
Style.AllowAllConstructorInitializersOnNextLine)
State.Stack.back().BreakBeforeParameter = false;
```
https://reviews.llvm.org/D40988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
