[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Russell McClellan via Phabricator via cfe-commits]

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