Clang-format: add finer-grained options for putting all arguments on one line

[russellmcc]

Summary

Add two new options,
AllowAllArgumentsOnNextLine and
AllowAllConstructorInitializersOnNextLine. These mirror the existing
AllowAllParametersOfDeclarationOnNextLine and allow me to support an
internal style guide where I work. I think this would be generally
useful, some have asked for it on stackoverflow:

https://stackoverflow.com/questions/30057534/clang-format-binpackarguments-not-working-as-expected

https://stackoverflow.com/questions/38635106/clang-format-how-to-prevent-all-function-arguments-on-next-line

[russellmcc]

ping! Thanks for your consideration

[uohcsemaj]

Ping! Would really love to use this tool with the newly added params.

[djasper]

Generally, upload patches with the full file as diff context. Phabricator hides that by default, but enables me to expand beyond the patch regions (where it currently says "Context not available").

lib/Format/ContinuationIndenter.cpp

756 ↗

(On Diff #126061)

Hm, this looks suspicious. Doesn't this mean that AllowAllArgumentsOnNextLine implies/overwrites AllowAllParametersOfDeclarationOnNextLine?

Now, there are two ways out of this:

• Fix it
• Provide different options

The question is whether someone would ever want AllowAllArgumentsOnNextLine to be false while AllowAllParametersOfDeclarationOnNextLine is true. That would seem weird to me. If not, it might be better to turn the existing option into an enum with three values (None, Declarations, All) or something.

962 ↗

(On Diff #126061)

I am not 100%, but I think this is doing too much. In accordance with the other options, this should still allow:

Constructor() : a(a), b(b) {}

so long as everything fits on one line (and I think it might not). Either way, add a test.

lib/Format/Format.cpp

748 ↗

(On Diff #126061)

I think there is no need to set these here and below. Everything derives from LLVM style.

unittests/Format/FormatTest.cpp

3440 ↗

(On Diff #126061)

If we go forward with this, the name of this option gets really confusing and we need to think about renaming it. Or maybe we can also turn it into an enum with three values?

Here also, the combination of

ConstructorInitializerAllOnOneLineOrOnePerLine=false and 
AllowAllConstructorInitializersOnNextLine=true

seems really weird. I wouldn't even know what the desired behavior is in that case.

3455 ↗

(On Diff #126061)

Remove this line.

[russellmcc]

Responded to review feedback:

• Fix bug where AllowAllArgumentsOnNextLine overrode AllowAllParametersOfDeclarationOnNextLine
• Add tests demonstrating independence of AllowAllArgumentsOnNextLine and AllowAllParametersOfDeclarationOnNextLine
• Add test showing AllowAllConstructorInitializersOnNextLine doesn't affect single-line constructors
• Removed unnecessary re-setting of llvm styles
• Removed bonus line from test

[russellmcc]

Thanks for the feedback! I'm very motivated to get some support for these features since they are required for my style guide. Let me know if my recent changes made sense to you.

lib/Format/ContinuationIndenter.cpp

756 ↗

(On Diff #126061)

Oops! Thanks for finding this. I think since other options are exposed separately for parameters and arguments (e.g., bin packing), it's more consistent to break these out separately.

962 ↗

(On Diff #126061)

I think the case you're talking about actually works; I've added a test.

lib/Format/Format.cpp

748 ↗

(On Diff #126061)

Removed!

unittests/Format/FormatTest.cpp

3440 ↗

(On Diff #126061)

I agree this combination is weird, however the situation already existed with declaration parameters (i.e., AllowAllParametersOfDeclarationOnNextLine has no effect when BinPackParameters was true). I think exposing these new parameters in this way is the most consistent with the existing approach.

I've edited the doc comment for AllowAllConstructorInitializersOnNextLine to note that it has no effect when ConstructorInitializerAllOnOneLineOrOnePerLine is false.

[russellmcc]

Ping! I believe all feedback has been addressed - further consideration would be much appreciated.

[russellmcc]

Bumping again - thanks so much for your time!

[uohcsemaj]

Ping!

[russellmcc]

Bump!

[russellmcc]

Bump again! Any feedback would be quite appreciated.

[uohcsemaj]

ping @djasper can we make a decision here this patch? I would really like to use these new styles :(

[djasper]

include/clang/Format/Format.h

154 ↗

(On Diff #133280)

writing just "initializer list" is confusing, especially next to the constructor initializer list below. Maybe "brace initializer list"?

Also, if this influences initializer lists and template argument lists, please add tests for those.

If you change this file, please run docs/tools/dump_format_style.py to update the docs.

(also, why is this comment so narrow?)

171 ↗

(On Diff #133280)

I think this comment is a bit confusing. The "initializer list" does fit on one line.

lib/Format/ContinuationIndenter.cpp

757 ↗

(On Diff #133280)

nitpick: move this up one line so it's next to the case for AllowAllParametersOfDeclarationOnNextLine.

unittests/Format/FormatTest.cpp

3438 ↗

(On Diff #133280)

Only testing this for BCIS_BeforeComma seems a bit bad to me. Is there a reason for it?

Also, I think we should have a test where the constructor declaration itself does not fit on one line, e.g. what's the behavior for:

Constructor(int param1,
            ...
            int paramN) {
    : aaaaaa(a), bbbbb(b) { .. }

[russellmcc]

Responded to review feedback.

Improved documentation comments for new options.

While adding tests for the other constructor line break styles, I noticed an inconsistency with the way "after colon" behaved with ConstructorInitializerAllOnOneLineOrOnePerLine. Unlike the other two modes, "after colon" breaking would never allow all initializers on a single line. I've changed the behavior so the new AllowAllConstructorInitializersOnNextLine is honored even in break after colon mode. This does cause a slight change in behavior for default settings, as now in after colon mode it will be allowed to put all the initializers in one line with default settings. I've updated the unit tests for break after colon mode to address this.

[russellmcc]

Further update doc comments, render rst file

[russellmcc]

Okay; I think I've responded to all feedback at this point. Thanks for your patience guiding me through my first contribution to this project. Let me know what else I can do to help get this merged!

[russellmcc]

Bump! Thanks for your consideration

[djasper]

lib/Format/ContinuationIndenter.cpp

760 ↗

(On Diff #144280)

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.

[russellmcc]

lib/Format/ContinuationIndenter.cpp

760 ↗

(On Diff #144280)

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;

[russellmcc]

Bump! Thanks again for your feedback.

[russellmcc]

Bump!

Thanks again for your time.

[djasper]

lib/Format/ContinuationIndenter.cpp

760 ↗

(On Diff #144280)

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 :( ).

unittests/Format/FormatTest.cpp

3444 ↗

(On Diff #144280)

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
}

3444 ↗

(On Diff #144280)

Err.. The second line inside the for-loop was meant to read:

Style.AllowAllArgumentsOnNextLine = i & 2;

[russellmcc]

Added suggested for loops to the test

[russellmcc]

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.

lib/Format/ContinuationIndenter.cpp

760 ↗

(On Diff #144280)

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.

unittests/Format/FormatTest.cpp

3444 ↗

(On Diff #144280)

How does this look? Your suggestions indeed added new coverage so I think that's helpful.

[russellmcc]

Bump! Thanks for your time!

[russellmcc]

Bump! Thanks for your time

[russellmcc]

Bump! Still looking for feedback on the latest round of changes.

[russellmcc]

bump! Thanks for your consideration.

[russellmcc]

Bump! Thanks again for your consideration.

[russellmcc]

Bump! Thanks again for your consideration

[russellmcc]

Bump! Thanks again for your time. As far as I can tell, it's ready for another round of review!

[russellmcc]

Ping! Thanks for your consideration. I'm still quite motivated to land this, please let me know if there's anything I can do, or if it's an unwanted patch.

[lebedev.ri]

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options comes into mind.

[russellmcc]

Thanks for the feedback! Does this mean that this won't be accepted? In my opinion, without these extra options, AllowAllParametersOfDeclarationOnNextLine is a very strange option. I don't think I'm the only one who feels this way, based on the stack overflow questions linked in the description. However, I fully understand if these changes are not wanted.

[russellmcc]

Bump! Thanks for the consideration.

[aol-nnov]

would love such a feature too!

also, there is kind of similar review https://reviews.llvm.org/D14484?id=39647 (by @djasper , also rejected. which is really sad)

[russellmcc]

Bump! Still watching this space - thanks so much for your time!

[russellmcc]

Bump! Would really appreciate any feedback on this!

[russellmcc]

Bump! Still looking for help here - thanks for the consideration.

[MyDeveloperDay]

So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.

Was there anything you think people were objecting too other than the normal "its a high bar to get in" and "its complex in here"?

I think its fairer to the person submitting a revision if they don't want feature in, we should give feedback as to why, but are we to assume silence is acceptance? how long do we wait? who is the decision maker?

I've personally never met an Engineer who didn't like having more knobs to fiddle with so I don't believe the rational that having more options is bad as long as they don't interfere with each other, for that there is the "Beyonce rule", if adding an option breaks someone else then "if they liked it they should have put a test on it!"

As far as I can tell this LGTM (I'm not the code owner, just someone wanting to help)

In the meantime I have uploaded this patch to my fork, where I'm maintaining a clang-format with revisions that seem ok, but get stalled in the review process.

https://github.com/mydeveloperday/clang-experimental

[klimek]

In D40988#1430502, @MyDeveloperDay wrote:

So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.

Was there anything you think people were objecting too other than the normal "its a high bar to get in" and "its complex in here"?

I think its fairer to the person submitting a revision if they don't want feature in, we should give feedback as to why, but are we to assume silence is acceptance? how long do we wait? who is the decision maker?

I've personally never met an Engineer who didn't like having more knobs to fiddle with so I don't believe the rational that having more options is bad as long as they don't interfere with each other, for that there is the "Beyonce rule", if adding an option breaks someone else then "if they liked it they should have put a test on it!"

As far as I can tell this LGTM (I'm not the code owner, just someone wanting to help)

In the meantime I have uploaded this patch to my fork, where I'm maintaining a clang-format with revisions that seem ok, but get stalled in the review process.

https://github.com/mydeveloperday/clang-experimental

I think in this case there were no objections to the knob, but djasper had concerns about the code.
The problem in this case is that there was a 3 month pause between the review comment and the response, and for those intricate changes (and yes, clang-format's design is problematic here in that it makes these changes really hard to get right) it take a long time to think oneself into the problem to fully understand where it could go wrong.

Regarding the abstract argument brought up about tests - this is not about whether specific things break, but whether the code gets harder to maintain over time, which in the end will lead to nobody being able to make changes, regardless whether there is a reviewer or not - I think it's important we don't underestimate that risk & cost.

[MyDeveloperDay]

nobody being able to make changes

Nobody IS able to make changes, but not because of complexity!

We discourage away potential contributors/maintainers by leaving their reviews for weeks/months/years, not just not letting them in, not even discussing them..

Most reviews are met with a sharp intake of breath, and a general comment about "how its a pretty big bar we have here and how complex it is and perhaps you shouldn't come in", rather than seeing it as a positive move forward in the right direction and a chance to bring someone on as a new contributor.

But we can't do anything to improve the complexity of the code if the changes to refactor code to be less complex is going to sit un-reviewed.

But I'm super glad that me giving an Accept and LGTM after 6 months is catching peoples attention... job done.

[russellmcc]

Ping! Still looking for help on this - I definitely don't want to diminish the complexity of this code, and would really appreciate any help getting this in. I've already apologized for the gap from feedback in July 2018 to response in October - and I'm happy to again - unfortunately sometimes life gets in the way! Please let me know if there's anything I can do to get this patch landed. As far as I understand, all outstanding comments on the code have been addressed.

[jkorous]

@russellmcc the patch has been approved by @MyDeveloperDay
https://reviews.llvm.org/D40988#1430502

Do you mean you don't have commit access and need someone to land the patch on your behalf?

[russellmcc]

Yes, I don’t have commit access and would need someone else to land the patch. Thanks!

[russellmcc]

@djasper Do you have any further concerns about this patch?

[russellmcc]

@MyDeveloperDay Thanks for the approve! I'm not sure what the lingering concerns are, as far as I know there have been no concerns since october of last year.

cfe/trunk/docs/ClangFormatStyleOptions.rst

**AlignTrailingComments** (``bool``)
   If ``true``, aligns trailing comments.
 
   .. code-block:: c++
 
     true:                                   false:
     int a;     // My comment a      vs.     int a; // My comment a
     int b = 2; // comment  b                int b = 2; // comment about b
 
+**AllowAllArgumentsOnNextLine** (``bool``)
+  If a function call or braced initializer list doesn't fit on a
+  line, allow putting all arguments onto the next line, even if
+  ``BinPackArguments`` is ``false``.
+
+  .. code-block:: c++
+
+    true:
+    callFunction(
+        a, b, c, d);
+
+    false:
+    callFunction(a,
+                 b,
+                 c,
+                 d);
+
+**AllowAllConstructorInitializersOnNextLine** (``bool``)
+  If a constructor definition with a member initializer list doesn't
+  fit on a single line, allow putting all member initializers onto the next
+  line, if ```ConstructorInitializerAllOnOneLineOrOnePerLine``` is true.
+  Note that this parameter has no effect if
+  ```ConstructorInitializerAllOnOneLineOrOnePerLine``` is false.
+
+  .. code-block:: c++
+
+    true:
+    MyClass::MyClass() :
+        member0(0), member1(2) {}
+
+    false:
+    MyClass::MyClass() :
+        member0(0),
+        member1(2) {}
+
 **AllowAllParametersOfDeclarationOnNextLine** (``bool``)
   If the function declaration doesn't fit on a line,
   allow putting all parameters of a function declaration onto
   the next line even if ``BinPackParameters`` is ``false``.
 
   .. code-block:: c++
 
     true:

cfe/trunk/include/clang/Format/Format.h

struct FormatStyle {
   /// If ``true``, aligns trailing comments.
   /// \code
   ///   true:                                   false:
   ///   int a;     // My comment a      vs.     int a; // My comment a
   ///   int b = 2; // comment  b                int b = 2; // comment about b
   /// \endcode
   bool AlignTrailingComments;
 
+  /// \brief If a function call or braced initializer list doesn't fit on a
+  /// line, allow putting all arguments onto the next line, even if
+  /// ``BinPackArguments`` is ``false``.
+  /// \code
+  ///   true:
+  ///   callFunction(
+  ///       a, b, c, d);
+  ///
+  ///   false:
+  ///   callFunction(a,
+  ///                b,
+  ///                c,
+  ///                d);
+  /// \endcode
+  bool AllowAllArgumentsOnNextLine;
+
+  /// \brief If a constructor definition with a member initializer list doesn't
+  /// fit on a single line, allow putting all member initializers onto the next
+  /// line, if ```ConstructorInitializerAllOnOneLineOrOnePerLine``` is true.
+  /// Note that this parameter has no effect if
+  /// ```ConstructorInitializerAllOnOneLineOrOnePerLine``` is false.
+  /// \code
+  ///   true:
+  ///   MyClass::MyClass() :
+  ///       member0(0), member1(2) {}
+  ///
+  ///   false:
+  ///   MyClass::MyClass() :
+  ///       member0(0),
+  ///       member1(2) {}
+  bool AllowAllConstructorInitializersOnNextLine;
+
   /// If the function declaration doesn't fit on a line,
   /// allow putting all parameters of a function declaration onto
   /// the next line even if ``BinPackParameters`` is ``false``.
   /// \code
   ///   true:
   ///   void myFunction(
   ///       int a, int b, int c, int d, int e);
   ///

   bool operator==(const FormatStyle &R) const {
     return AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
            AlignConsecutiveAssignments == R.AlignConsecutiveAssignments &&
            AlignConsecutiveDeclarations == R.AlignConsecutiveDeclarations &&
            AlignEscapedNewlines == R.AlignEscapedNewlines &&
            AlignOperands == R.AlignOperands &&
            AlignTrailingComments == R.AlignTrailingComments &&
+           AllowAllArgumentsOnNextLine == R.AllowAllArgumentsOnNextLine &&
+           AllowAllConstructorInitializersOnNextLine ==
+               R.AllowAllConstructorInitializersOnNextLine &&
            AllowAllParametersOfDeclarationOnNextLine ==
                R.AllowAllParametersOfDeclarationOnNextLine &&
            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==

cfe/trunk/lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
   // If we break after { or the [ of an array initializer, we should also break
   // before the corresponding } or ].
   if (PreviousNonComment &&
       (PreviousNonComment->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
        opensProtoMessageField(*PreviousNonComment, Style)))
     State.Stack.back().BreakBeforeClosingBrace = true;
 
   if (State.Stack.back().AvoidBinPacking) {
-    // If we are breaking after '(', '{', '<', this is not bin packing
-    // unless AllowAllParametersOfDeclarationOnNextLine is false or this is a
-    // dict/object literal.
-    if (!Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
+    // 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) ||
         (!Style.AllowAllParametersOfDeclarationOnNextLine &&
          State.Line->MustBeDeclaration) ||
+        (!Style.AllowAllArgumentsOnNextLine &&
+         !State.Line->MustBeDeclaration) ||
+        (!Style.AllowAllConstructorInitializersOnNextLine &&
+         PreviousIsBreakingCtorInitializerColon) ||
         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;
   }
 
   return Penalty;
 }
 
 unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
   if (!State.NextToken || !State.NextToken->Previous)
     return 0;
if (Current.is(TT_CtorInitializerColon) &&
     //       Next(...)
     //       ^ line up here.
     State.Stack.back().Indent =
         State.Column +
         (Style.BreakConstructorInitializers == FormatStyle::BCIS_BeforeComma
              ? 0
              : 2);
     State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
-    if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
+    if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
       State.Stack.back().AvoidBinPacking = true;
+      State.Stack.back().BreakBeforeParameter =
+          !Style.AllowAllConstructorInitializersOnNextLine;
+    } else {
       State.Stack.back().BreakBeforeParameter = false;
     }
+  }
   if (Current.is(TT_CtorInitializerColon) &&
       Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon) {
     State.Stack.back().Indent =
         State.FirstIndent + Style.ConstructorInitializerIndentWidth;
     State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
     if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
       State.Stack.back().AvoidBinPacking = true;
   }

cfe/trunk/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AlignAfterOpenBracket", Style.AlignAfterOpenBracket);
     IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
+    IO.mapOptional("AllowAllArgumentsOnNextLine",
+                   Style.AllowAllArgumentsOnNextLine);
+    IO.mapOptional("AllowAllConstructorInitializersOnNextLine",
+                   Style.AllowAllConstructorInitializersOnNextLine);
     IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
                    Style.AllowAllParametersOfDeclarationOnNextLine);
     IO.mapOptional("AllowShortBlocksOnASingleLine",
                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",
                    Style.AllowShortIfStatementsOnASingleLine);
     IO.mapOptional("AllowShortLoopsOnASingleLine",
                    Style.AllowShortLoopsOnASingleLine);
     IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
                    Style.AlwaysBreakAfterDefinitionReturnType);
     IO.mapOptional("AlwaysBreakAfterReturnType",
                    Style.AlwaysBreakAfterReturnType);
+
     // If AlwaysBreakAfterDefinitionReturnType was specified but
     // AlwaysBreakAfterReturnType was not, initialize the latter from the
     // former for backwards compatibility.
     if (Style.AlwaysBreakAfterDefinitionReturnType != FormatStyle::DRTBS_None &&
         Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_None) {
       if (Style.AlwaysBreakAfterDefinitionReturnType == FormatStyle::DRTBS_All)
         Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_AllDefinitions;
       else if (Style.AlwaysBreakAfterDefinitionReturnType ==
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignOperands = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
+  LLVMStyle.AllowAllArgumentsOnNextLine = true;
+  LLVMStyle.AllowAllConstructorInitializersOnNextLine = true;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConstructorInitializers) {
   EXPECT_EQ("Constructor()\n"
             "    : // Comment forcing unwanted break.\n"
             "      aaaa(aaaa) {}",
             format("Constructor() :\n"
                    "    // Comment forcing unwanted break.\n"
                    "    aaaa(aaaa) {}"));
 }
 
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  Style.BinPackParameters = false;
+
+  for (int i = 0; i < 4; ++i) {
+    // Test all combinations of parameters that should not have an effect.
+    Style.AllowAllParametersOfDeclarationOnNextLine = i & 1;
+    Style.AllowAllArgumentsOnNextLine = i & 2;
+
+    Style.AllowAllConstructorInitializersOnNextLine = true;
+    Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+    verifyFormat("Constructor()\n"
+                 "    : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+    verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+    Style.AllowAllConstructorInitializersOnNextLine = false;
+    verifyFormat("Constructor()\n"
+                 "    : aaaaaaaaaaaaaaaaaaaa(a)\n"
+                 "    , bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+    verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+    Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+    Style.AllowAllConstructorInitializersOnNextLine = true;
+    verifyFormat("Constructor()\n"
+                 "    : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+
+    Style.AllowAllConstructorInitializersOnNextLine = false;
+    verifyFormat("Constructor()\n"
+                 "    : aaaaaaaaaaaaaaaaaaaa(a),\n"
+                 "      bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+
+    Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+    Style.AllowAllConstructorInitializersOnNextLine = true;
+    verifyFormat("Constructor() :\n"
+                 "    aaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+
+    Style.AllowAllConstructorInitializersOnNextLine = false;
+    verifyFormat("Constructor() :\n"
+                 "    aaaaaaaaaaaaaaaaaa(a),\n"
+                 "    bbbbbbbbbbbbbbbbbbbbb(b) {}",
+                 Style);
+  }
+
+  // Test interactions between AllowAllParametersOfDeclarationOnNextLine and
+  // AllowAllConstructorInitializersOnNextLine in all
+  // BreakConstructorInitializers modes
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbbb)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a)\n"
+               "    , bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb,\n"
+               "    int cccccccccccccccc)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a)\n"
+               "    , bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbbb)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a),\n"
+               "      bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb,\n"
+               "    int cccccccccccccccc)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb)\n"
+               "    : aaaaaaaaaaaaaaaaaaaa(a),\n"
+               "      bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbbb) :\n"
+               "    aaaaaaaaaaaaaaaaaaaa(a),\n"
+               "    bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb,\n"
+               "    int cccccccccccccccc) :\n"
+               "    aaaaaaaaaaaaaaaaaaaa(a), bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbbb) :\n"
+               "    aaaaaaaaaaaaaaaaaaaa(a),\n"
+               "    bbbbbbbbbbbbbbbbbbbbb(b) {}",
+               Style);
+}
+
+TEST_F(FormatTest, AllowAllArgumentsOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 60;
+  Style.BinPackArguments = false;
+  for (int i = 0; i < 4; ++i) {
+    // Test all combinations of parameters that should not have an effect.
+    Style.AllowAllParametersOfDeclarationOnNextLine = i & 1;
+    Style.AllowAllConstructorInitializersOnNextLine = i & 2;
+
+    Style.AllowAllArgumentsOnNextLine = true;
+    verifyFormat("void foo() {\n"
+                 "  FunctionCallWithReallyLongName(\n"
+                 "      aaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbb);\n"
+                 "}",
+                 Style);
+    Style.AllowAllArgumentsOnNextLine = false;
+    verifyFormat("void foo() {\n"
+                 "  FunctionCallWithReallyLongName(\n"
+                 "      aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+                 "      bbbbbbbbbbbb);\n"
+                 "}",
+                 Style);
+
+    Style.AllowAllArgumentsOnNextLine = true;
+    verifyFormat("void foo() {\n"
+                 "  auto VariableWithReallyLongName = {\n"
+                 "      aaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbb};\n"
+                 "}",
+                 Style);
+    Style.AllowAllArgumentsOnNextLine = false;
+    verifyFormat("void foo() {\n"
+                 "  auto VariableWithReallyLongName = {\n"
+                 "      aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+                 "      bbbbbbbbbbbb};\n"
+                 "}",
+                 Style);
+  }
+
+  // This parameter should not affect declarations.
+  Style.BinPackParameters = false;
+  Style.AllowAllArgumentsOnNextLine = false;
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("void FunctionCallWithReallyLongName(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaa, int bbbbbbbbbbbb);",
+               Style);
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  verifyFormat("void FunctionCallWithReallyLongName(\n"
+               "    int aaaaaaaaaaaaaaaaaaaaaaa,\n"
+               "    int bbbbbbbbbbbb);",
+               Style);
+}
+
 TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
 
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}",
                getStyleWithColumns(Style, 45));
   verifyFormat("Constructor() :\n"
                "    Initializer(FitsOnTheLine) {}",
                getStyleWithColumns(Style, 44));
   verifyFormat("Constructor() :\n"
                "    Initializer(FitsOnTheLine) {}",
                getStyleWithColumns(Style, 43));
 
   verifyFormat("template <typename T>\n"
                "Constructor() : Initializer(FitsOnTheLine) {}",
                getStyleWithColumns(Style, 50));
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  verifyFormat(
+      "SomeClass::Constructor() :\n"
+      "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
+      Style);
 
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   verifyFormat(
       "SomeClass::Constructor() :\n"
       "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
-    Style);
+      Style);
 
   verifyFormat(
       "SomeClass::Constructor() :\n"
       "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
       "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}",
-    Style);
+      Style);
   verifyFormat(
       "SomeClass::Constructor() :\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
       "    aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
     Style);
   verifyFormat("Constructor(aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
                "            aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) :\n"
                "    aaaaaaaaaa(aaaaaa) {}",
TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
   verifyFormat("Constructor() :\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaa),\n"
                "    aaaaaaaaaaa(aaaaaaaaaaa),\n"
                "    aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}",
          Style);
 
   FormatStyle OnePerLine = Style;
   OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
-  OnePerLine.AllowAllParametersOfDeclarationOnNextLine = false;
+  OnePerLine.AllowAllConstructorInitializersOnNextLine = false;
   verifyFormat("SomeClass::Constructor() :\n"
                "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
                "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
                "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}",
                OnePerLine);
   verifyFormat("SomeClass::Constructor() :\n"
                "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa), // Some comment\n"
                "    aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"

 
 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE_BOOL(AlignOperands);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
+  CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
+  CHECK_PARSE_BOOL(AllowAllConstructorInitializersOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);