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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356834: Clang-format: add finer-grained options for putting 
all arguments on one line (authored by paulhoad, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D40988?vs=169288=191994#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988

Files:
  cfe/trunk/docs/ClangFormatStyleOptions.rst
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -3956,6 +3956,191 @@
"() {}"));
 }
 
+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"
+ ": (a), b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a),\n"
+ "  b(b) {}",
+ Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor() :\n"
+ "aa(a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor() :\n"
+ "aa(a),\n"
+ "b(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 , int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+

[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.

@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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Would really appreciate any feedback on this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrey via Phabricator via cfe-commits]

aol-nnov added a comment.

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)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks for the consideration.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks again for your consideration


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks again for your consideration.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

bump!  Thanks for your consideration.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks for your time


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks for your time!


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 = 
> 

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 169288.
russellmcc added a comment.

Added suggested for loops to the test


https://reviews.llvm.org/D40988

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3808,6 +3808,191 @@
"() {}"));
 }
 
+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"
+ ": (a), b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a),\n"
+ "  b(b) {}",
+ Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor() :\n"
+ "aa(a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor() :\n"
+ "aa(a),\n"
+ "b(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 , int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  

[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: unittests/Format/FormatTest.cpp:3444
+
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",

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 , int b)\n"
>  ": (a), b(b) {}",
>  Style);
> // ... more tests
> 
> 
> Style.AllowAllConstructorInitializersOnNextLine = false;
> verifyFormat("SomeClassWithALongName::Constructor(\n"
>  "int , int b)\n"
>  ": (a)\n"
>  ", b(b) {}",
>  Style);
> // ... more tests
>   }
Err.. The second line inside the for-loop was meant to read:

  Style.AllowAllArgumentsOnNextLine = i & 2;


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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
+
+  

[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.

Bump!

Thanks again for your time.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks again for your feedback.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 &&

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.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!  Thanks for your consideration


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc marked an inline comment as not done.
russellmcc added a comment.

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!


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 144280.
russellmcc marked an inline comment as done.
russellmcc added a comment.

Further update doc comments, render rst file


https://reviews.llvm.org/D40988

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,168 @@
"() {}"));
 }
 
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  Style.BinPackParameters = false;
+
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor() :\n"
+   "aa(a), b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor() :\n"
+   "aa(a),\n"
+   "b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b) :\n"
+   "(a),\n"
+   "b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 144269.
russellmcc added a comment.

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.


https://reviews.llvm.org/D40988

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,154 @@
"() {}"));
 }
 
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  Style.BinPackParameters = false;
+
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int , int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b,\n"
+   "int )\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("SomeClassWithALongName::Constructor(\n"
+   "int ,\n"
+   "int b)\n"
+   ": (a),\n"
+   "  b(b) {}",
+   Style);
+
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor() :\n"
+   "aa(a), b(b) {}",
+ 

[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: include/clang/Format/Format.h:154
 
+  /// \brief If a function call, initializer list, or template
+  /// argument list doesn't fit on a line, allow putting all

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?)



Comment at: include/clang/Format/Format.h:171
+
+  /// \brief If a constructor initializer list doesn't fit on a line, allow
+  /// putting all initializers onto the next line, if

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



Comment at: lib/Format/ContinuationIndenter.cpp:757
+Previous.is(TT_DictLiteral) ||
+(!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration))
   State.Stack.back().BreakBeforeParameter = true;

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



Comment at: unittests/Format/FormatTest.cpp:3438
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;

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) {
  : aa(a), b(b) { .. }


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Chou via Phabricator via cfe-commits]

uohcsemaj added a comment.

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


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump again!  Any feedback would be quite appreciated.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bump!


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Chou via Phabricator via cfe-commits]

uohcsemaj added a comment.

Ping!


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Bumping again - thanks so much for your time!


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

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.




Comment at: lib/Format/ContinuationIndenter.cpp:756
  State.Line->MustBeDeclaration) ||
-Previous.is(TT_DictLiteral))
+Previous.is(TT_DictLiteral) || !Style.AllowAllArgumentsOnNextLine)
   State.Stack.back().BreakBeforeParameter = true;

djasper wrote:
> 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.
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.



Comment at: lib/Format/ContinuationIndenter.cpp:962
 State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
-if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
+if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
   State.Stack.back().AvoidBinPacking = true;

djasper wrote:
> 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.
I think the case you're talking about actually works; I've added a test.



Comment at: lib/Format/Format.cpp:748
   } else {
+ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ChromiumStyle.AllowAllConstructorInitializersOnNextLine = true;

djasper wrote:
> I think there is no need to set these here and below. Everything derives from 
> LLVM style.
Removed!



Comment at: unittests/Format/FormatTest.cpp:3440
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;

djasper wrote:
> 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.
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.



https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 133280.
russellmcc added a comment.

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


https://reviews.llvm.org/D40988

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,56 @@
"() {}"));
 }
 
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+  verifyFormat("Constructor() : a(a), b(b) {}", Style);
+}
+
+TEST_F(FormatTest, AllowAllArgumentsOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 60;
+  Style.BinPackArguments = false;
+  Style.AllowAllArgumentsOnNextLine = true;
+  verifyFormat("void foo() {\n"
+   "  FunctionCallWithReallyLongName(\n"
+   "  aaa, );\n"
+   "}",
+   Style);
+  Style.AllowAllArgumentsOnNextLine = false;
+  verifyFormat("void foo() {\n"
+   "  FunctionCallWithReallyLongName(\n"
+   "  aaa,\n"
+   "  );\n"
+   "}",
+   Style);
+
+  // This parameter should not affect declarations.
+  Style.BinPackParameters = false;
+  Style.AllowAllParametersOfDeclarationOnNextLine = true;
+  verifyFormat("void FunctionCallWithReallyLongName(\n"
+   "int aaa, int );",
+   Style);
+  Style.AllowAllParametersOfDeclarationOnNextLine = false;
+  verifyFormat("void FunctionCallWithReallyLongName(\n"
+   "int aaa,\n"
+   "int );",
+   Style);
+}
+
 TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
@@ -10020,6 +10070,8 @@
   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);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -287,6 +287,10 @@
 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",
@@ -303,6 +307,7 @@
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakAfterReturnType",
Style.AlwaysBreakAfterReturnType);
+
 // If AlwaysBreakAfterDefinitionReturnType was specified but
 // AlwaysBreakAfterReturnType was not, initialize the latter from the
 // former for backwards compatibility.
@@ -575,6 +580,8 @@
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
+  LLVMStyle.AllowAllArgumentsOnNextLine = true;
+  

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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").




Comment at: lib/Format/ContinuationIndenter.cpp:756
  State.Line->MustBeDeclaration) ||
-Previous.is(TT_DictLiteral))
+Previous.is(TT_DictLiteral) || !Style.AllowAllArgumentsOnNextLine)
   State.Stack.back().BreakBeforeParameter = true;

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.



Comment at: lib/Format/ContinuationIndenter.cpp:962
 State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
-if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
+if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
   State.Stack.back().AvoidBinPacking = true;

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.



Comment at: lib/Format/Format.cpp:748
   } else {
+ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ChromiumStyle.AllowAllConstructorInitializersOnNextLine = true;

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



Comment at: unittests/Format/FormatTest.cpp:3440
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;

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.



Comment at: unittests/Format/FormatTest.cpp:3455
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;

Remove this line.


Repository:
  rC Clang

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Chou via Phabricator via cfe-commits]

uohcsemaj added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

ping!  Thanks for your consideration


Repository:
  rC Clang

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc created this revision.
Herald added a subscriber: klimek.

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


Repository:
  rC Clang

https://reviews.llvm.org/D40988

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,43 @@
"() {}"));
 }
 
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.AllowAllConstructorInitializersOnNextLine = true;
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",
+   Style);
+
+  Style.AllowAllConstructorInitializersOnNextLine = false;
+  verifyFormat("Constructor()\n"
+   ": (a)\n"
+   ", b(b) {}",
+   Style);
+}
+
+TEST_F(FormatTest, AllowAllArgumentsOnNextLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
+  Style.BinPackArguments = false;
+  Style.AllowAllArgumentsOnNextLine = true;
+  verifyFormat("void foo() {\n"
+   "  FunctionCallWithReallyLongName(\n"
+   "  aaa, );\n"
+   "}",
+   Style);
+  Style.AllowAllArgumentsOnNextLine = false;
+  verifyFormat("void foo() {\n"
+   "  FunctionCallWithReallyLongName(\n"
+   "  aaa,\n"
+   "  );\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
@@ -10020,6 +10057,8 @@
   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);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -287,6 +287,10 @@
 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",
@@ -303,6 +307,7 @@
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakAfterReturnType",
Style.AlwaysBreakAfterReturnType);
+
 // If AlwaysBreakAfterDefinitionReturnType was specified but
 // AlwaysBreakAfterReturnType was not, initialize the latter from the
 // former for backwards compatibility.
@@ -575,6 +580,8 @@
   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;
@@ -738,6 +745,8 @@
 ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
 ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
+ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+