djasper added inline comments.
================
Comment at: lib/Format/ContinuationIndenter.cpp:760
(!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration) ||
+ (!Style.AllowAllArgumentsOnNextLine &&
----------------
russellmcc wrote:
> 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;
> ```
I find it hard to say whether you actually have a test for this. I'll make a
suggestion on how to make these tests more maintainable below.
I understand now how this works, but the specific case I was worried about is:
AllowAllConstructorInitializersOnNextLine = true
AllowAllArgumentsOnNextLine = false
AllowAllParametersOfDeclarationOnNextLine = false
(likely you only need one of the latter, but I am not sure which one :) )
This works, because you are overwriting the value again in the subsequent if
statement (which I hadn't seen).
However, I do personally find that logic hard to reason about (if you have a
sequence of if statements where some of them might overwrite the same value).
Fundamentally, you are doing:
if (something)
value = true;
if (otherthing)
value = false;
I think we don't care about (don't want to change) a pre-existing "value =
true" and so we actually just need:
if (something && !otherthing)
value = true;
Or am I missing something? If not, let's actually use the latter and simplify
the "something && !otherthing" (e.g. by pulling out variables/functions) until
it is readable again. Let me know if you want me to take a stab at that (I
promise it won't take weeks again :( ).
================
Comment at: unittests/Format/FormatTest.cpp:3444
+
+ verifyFormat("Constructor()\n"
+ " : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
----------------
I find these tests hard to read and reason about. How about writing them like
this:
for (int i = 0; i < 4; ++i) { // There might be a better way to iterate
// Test all combinations of parameters that should not have an effect.
Style.AllowAllParametersOfDeclarationOnNextLine = i & 1;
Style.AllowAllConstructorInitializersOnNextLine = i & 2;
Style.AllowAllConstructorInitializersOnNextLine = true;
verifyFormat("SomeClassWithALongName::Constructor(\n"
" int aaaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbbb)\n"
" : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
Style);
// ... more tests
Style.AllowAllConstructorInitializersOnNextLine = false;
verifyFormat("SomeClassWithALongName::Constructor(\n"
" int aaaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbbb)\n"
" : aaaaaaaaaaaaaaaaaaaa(a)\n"
" , bbbbbbbbbbbbbbbbbbbbb(b) {}",
Style);
// ... more tests
}
https://reviews.llvm.org/D40988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
