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

[Daniel Jasper via Phabricator via cfe-commits]

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