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

[Russell McClellan via Phabricator via cfe-commits]

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