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

[jtbandes]

Summary

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 😅

[jtbandes]

Not exactly sure who is the right person for this.

[djasper]

What happens if the function call where this happens actually does not have multiple parameters but one parameter with many operands, e.g. changing your test case to:

arg3____________________________ + is + quite + long + so + it
    + f(arguments << of << its << subexpressions << lengthy << as << they
                  << may << or__ << may)

lib/Format/ContinuationIndenter.cpp

924

I think you cannot get here if .size() is 0 as this is iterating over the elements.

925

LLVM coding style doesn't use braces for single-statement-ifs

unittests/Format/FormatTest.cpp

2596

Is this important?

2597

Does this bug only happen when breaking before operators? If not can you add a test case with None?

2599

This is not tested/changed at all, I think.

2600

Don't use raw strings to be more consistent with the rest. If we want, we should eventually convert all of the tests in one go.

Also, try to come up with a small reduced test case. Here you are actually testing much more than you want to test. Also, you set the ColumnLimit of Style in order to force earlier linebreaks.

[jtbandes]

Thanks for the feedback, I'll work on making those changes.

In D32475#738425, @djasper wrote:

What happens if the function call where this happens actually does not have multiple parameters but one parameter with many operands, e.g. changing your test case to:

arg3____________________________ + is + quite + long + so + it
    + f(arguments << of << its << subexpressions << lengthy << as << they
                  << may << or__ << may)

I don't think this is relevant to what I am trying to fix. The issue being fixed is specifically with multiple arguments: I don't want BinPackArguments=false to affect bin-packing of an individual argument. For completeness, though, I can include a test case with one argument.

lib/Format/ContinuationIndenter.cpp

924

Good point, thanks.

unittests/Format/FormatTest.cpp

2596

Not really. I can remove it.

[jtbandes]

lib/Format/ContinuationIndenter.cpp

924

Actually, I can probably just use *I > prec::Comma.

[djasper]

My point is though that even with only one argument, the BinPackArguments setting might lead to this bug. If not, that's good :).

[jtbandes]

unittests/Format/FormatTest.cpp

2597

Yeah it is only when breaking before operators, because the condition which causes mustBreak to be true includes Current.CanBreakBefore.

2599

That's a good point. I had this because my test code is a function call at the top level, which is treated as a signature rather than a call. I will wrap it in another block and remove the BinPackParameters setting.

[jtbandes]

Updates from review

[jtbandes]

@djasper how does this look?

I could try to simplify the examples further, but I feel it's important to have calls with many subexpressions to exercise this behavior properly. FWIW, my real-world use case is with << appearing as an output stream operator inside a macro invocation.

[jtbandes]

Friendly ping :) happy to make more changes if needed. Thanks again for your time.

[djasper]

Submitted as r302427.

lib/Format/ContinuationIndenter.cpp

for (SmallVectorImpl<prec::Level>::const_reverse_iterator
            E = Current.FakeLParens.rend();
        I != E; ++I) {
     ParenState NewParenState = State.Stack.back();
     NewParenState.ContainsLineBreak = false;
     NewParenState.LastOperatorWrapped = true;
     NewParenState.NoLineBreak =
         NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
+    // Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+    if (*I > 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.
     if (!Current.isTrailingComment() &&
         (Style.AlignOperands || *I < prec::Assignment) &&
         (!Previous || Previous->isNot(tok::kw_return) ||
          (Style.Language != FormatStyle::LK_Java && *I > 0)) &&
         (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign ||

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreakingBeforeNonAssigmentOperators) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
   verifyFormat("int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa =\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;",
                Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.ColumnLimit = 40;
+  verifyFormat("void test() {\n"
+               "  someFunction(\n"
+               "      this + argument + is + quite\n"
+               "      + long + so + it + gets + wrapped\n"
+               "      + but + remains + bin - packed);\n"
+               "}",
+               Style);
+  verifyFormat("void test() {\n"
+               "  someFunction(arg1,\n"
+               "               this + argument + is\n"
+               "                   + quite + long + so\n"
+               "                   + it + gets + wrapped\n"
+               "                   + but + remains + bin\n"
+               "                   - packed,\n"
+               "               arg3);\n"
+               "}",
+               Style);
+  verifyFormat("void test() {\n"
+               "  someFunction(\n"
+               "      arg1,\n"
+               "      this + argument + has\n"
+               "          + anotherFunc(nested,\n"
+               "                        calls + whose\n"
+               "                            + arguments\n"
+               "                            + are + also\n"
+               "                            + wrapped,\n"
+               "                        in + addition)\n"
+               "          + to + being + bin - packed,\n"
+               "      arg3);\n"
+               "}",
+               Style);
+
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat("void test() {\n"
+               "  someFunction(\n"
+               "      arg1,\n"
+               "      this + argument + has +\n"
+               "          anotherFunc(nested,\n"
+               "                      calls + whose +\n"
+               "                          arguments +\n"
+               "                          are + also +\n"
+               "                          wrapped,\n"
+               "                      in + addition) +\n"
+               "          to + being + bin - packed,\n"
+               "      arg3);\n"
+               "}",
+               Style);
+}
+
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
                getLLVMStyleWithColumns(45));
   verifyFormat("Constructor()\n"
                "    : Inttializer(FitsOnTheLine) {}",
                getLLVMStyleWithColumns(44));
   verifyFormat("Constructor()\n"