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.
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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits