[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