russellmcc added a comment.
Sorry for dropping this for so long! Stuff got busy at work and I've been
happily using my fork with this change for some time. I would really like this
to get in, and promise to be responsive to feedback.
================
Comment at: lib/Format/ContinuationIndenter.cpp:760
(!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration) ||
+ (!Style.AllowAllArgumentsOnNextLine &&
----------------
djasper wrote:
> 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 :( ).
First of all, I should admit I don't fully understand the full set of things
that modifies `BreakBeforeParameter`, it seems to be set from a lot of places.
Because I don't have this understanding, I'm trying to be very conservative.
That is, I want to modify it only when I know it doesn't match the user
preferences.
Doing as you suggest does break the tests, because someone else is setting
`BreakBeforeParameter` to `true` with `Style.BreakConstructorInitializers =
FormatStyle::BCIS_AfterColon` when we've done line breaking on the _parameter_
list of the constructor.
I haven't traced down exactly where this happens, since there are so many
potential suspects
However, to maintain what the user probably wants here, we need
`BreakBeforeParameter` to be *set* to false to actually put all the constructor
initializers on the same line. Simply not setting it to true isn't good enough.
================
Comment at: unittests/Format/FormatTest.cpp:3444
+
+ verifyFormat("Constructor()\n"
+ " : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
----------------
djasper wrote:
> djasper wrote:
> > 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
> > }
> Err.. The second line inside the for-loop was meant to read:
>
> Style.AllowAllArgumentsOnNextLine = i & 2;
How does this look? Your suggestions indeed added new coverage so I think
that's helpful.
https://reviews.llvm.org/D40988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
