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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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)



================
Comment at: lib/Format/ContinuationIndenter.cpp:923
+    // Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+    if (Current.FakeLParens.size() > 0 &&
+        Current.FakeLParens.back() > prec::Comma) {
----------------
I think you cannot get here if .size() is 0 as this is iterating over the elements.


================
Comment at: lib/Format/ContinuationIndenter.cpp:924
+    if (Current.FakeLParens.size() > 0 &&
+        Current.FakeLParens.back() > prec::Comma) {
+      NewParenState.AvoidBinPacking = false;
----------------
LLVM coding style doesn't use braces for single-statement-ifs


================
Comment at: unittests/Format/FormatTest.cpp:2596
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
----------------
Is this important?


================
Comment at: unittests/Format/FormatTest.cpp:2597
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
----------------
Does this bug only happen when breaking before operators? If not can you add a test case with None?


================
Comment at: unittests/Format/FormatTest.cpp:2599
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(
----------------
This is not tested/changed at all, I think.


================
Comment at: unittests/Format/FormatTest.cpp:2600
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(
+someFunction(
----------------
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.


https://reviews.llvm.org/D32475