[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