[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

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

This is an attempt to fix the issue described in my recent email: http://lists.llvm.org/pipermail/cfe-dev/2017-April/053632.html

After digging in for a while, I learned that:

- the spurious line breaks were occurring inside the condition `Current.is(TT_BinaryOperator) && Current.CanBreakBefore && State.Stack.back().BreakBeforeParameter`.
- `BreakBeforeParameter` was being set to true because `AvoidBinPacking` was true.
- `AvoidBinPacking` was true because it propagated all the way along the stack starting from the `(` "scope opener".

Given the above, it seemed to make sense to reset `AvoidBinPacking` to `false` when propagating it into a subexpression.

👋 hello @djasper — you seem to be the most prolific clang-formatter — please feel free to tear this apart 😅


https://reviews.llvm.org/D32475

Files:
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2591,6 +2591,30 @@
                Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(
+someFunction(
+    arg1,
+    arg2,
+    arg3____________________________ + is + quite + long + so + it
+        + f(and_even,
+            the,
+            arguments << of << its << subexpressions << lengthy << as << they
+                      << may << or__ << may,
+            not__,
+            be)
+        + a + r + e / l + o + v + i + n + g + l + y / b + i + n - p + a + c + k
+        + e + d,
+    arg4,
+    arg5);
+               )").trim(), Style);
+}
+
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
Index: lib/Format/ContinuationIndenter.cpp
===================================================================
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -919,6 +919,12 @@
     NewParenState.NoLineBreak =
         NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
+    // Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+    if (Current.FakeLParens.size() > 0 &&
+        Current.FakeLParens.back() > prec::Comma) {
+      NewParenState.AvoidBinPacking = false;
+    }
+
     // Indent from 'LastSpace' unless these are fake parentheses encapsulating
     // a builder type call after 'return' or, if the alignment after opening
     // brackets is disabled.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32475.96509.patch
Type: text/x-patch
Size: 1952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170425/941ee922/attachment.bin>