gn: Check if formatting a single line list to multiple reduces penalty
Previously, when formatting binary operators, gn format would only try
to break around the operators, but the determination of whether a list
was single or multiline was only determined by the number of elements
in the list (and in some cases, the LHS of the assignment).
Instead now, check if spliting a single line list into multiline would
avoid going past 80 columns.
A test run of running this new behaviour on all of chromium/src is at
https://chromium-review.googlesource.com/c/chromium/src/+/801660/, and
it seems like a universal improvement, fixing a number of misformattings.
Bug: 790021
Change-Id: I06ab6f06388dc9429a75407864769484d3fcea41
Reviewed-on: https://chromium-review.googlesource.com/801658
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#520726}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 7f86d891eda0a8a4207a7c65fd353702ff15c26cdiff --git a/tools/gn/command_format.cc b/tools/gn/command_format.cc
index ceb325d..5f9f6ed 100644
--- a/tools/gn/command_format.cc
+++ b/tools/gn/command_format.cc
@@ -576,18 +576,45 @@
sub2.Print(suffix);
penalty_next_line += AssessPenalty(sub2.String());
- // If in both cases it was forced past 80col, then we don't break to avoid
+ // Force a list on the RHS that would normally be a single line into
+ // multiline.
+ bool tried_rhs_multiline = false;
+ Printer sub3;
+ InitializeSub(&sub3);
+ int penalty_multiline_rhs_list = std::numeric_limits<int>::max();
+ const ListNode* rhs_list = binop->right()->AsList();
+ if (is_assignment && rhs_list &&
+ !ListWillBeMultiline(rhs_list->contents(), rhs_list->End())) {
+ sub3.Print(" ");
+ sub3.stack_.push_back(IndentState(start_column, false, false));
+ sub3.Sequence(kSequenceStyleList, rhs_list->contents(), rhs_list->End(),
+ true);
+ sub3.stack_.pop_back();
+ penalty_multiline_rhs_list = AssessPenalty(sub3.String());
+ tried_rhs_multiline = true;
+ }
+
+ // If in all cases it was forced past 80col, then we don't break to avoid
// breaking after '=' in the case of:
// variable = "... very long string ..."
// as breaking and indenting doesn't make things much more readable, even
- // though there's less characters past the maximum width.
- bool exceeds_maximum_either_way = ExceedsMaximumWidth(sub1.String()) &&
- ExceedsMaximumWidth(sub2.String());
+ // though there's fewer characters past the maximum width.
+ bool exceeds_maximum_all_ways =
+ ExceedsMaximumWidth(sub1.String()) &&
+ ExceedsMaximumWidth(sub2.String()) &&
+ (!tried_rhs_multiline || ExceedsMaximumWidth(sub3.String()));
if (penalty_current_line < penalty_next_line ||
- exceeds_maximum_either_way) {
+ exceeds_maximum_all_ways) {
Print(" ");
Expr(binop->right(), prec_right, std::string());
+ } else if (tried_rhs_multiline &&
+ penalty_multiline_rhs_list < penalty_next_line) {
+ // Force a multiline list on the right.
+ Print(" ");
+ stack_.push_back(IndentState(start_column, false, false));
+ Sequence(kSequenceStyleList, rhs_list->contents(), rhs_list->End(), true);
+ stack_.pop_back();
} else {
// Otherwise, put first argument and op, and indent next.
Newline();
diff --git a/tools/gn/command_format_unittest.cc b/tools/gn/command_format_unittest.cc
index f60f2b8..c0358fe 100644
--- a/tools/gn/command_format_unittest.cc
+++ b/tools/gn/command_format_unittest.cc
@@ -100,3 +100,4 @@
FORMAT_TEST(066)
FORMAT_TEST(067)
FORMAT_TEST(068)
+FORMAT_TEST(069)
diff --git a/tools/gn/format_test_data/069.gn b/tools/gn/format_test_data/069.gn
new file mode 100644
index 0000000..200b313
--- /dev/null
+++ b/tools/gn/format_test_data/069.gn
@@ -0,0 +1,3 @@
+if (true) {
+ configs -= [ "//third_party/mini_chromium/mini_chromium/build:Wexit_time_destructors", ]
+}
diff --git a/tools/gn/format_test_data/069.golden b/tools/gn/format_test_data/069.golden
new file mode 100644
index 0000000..64d4acc
--- /dev/null
+++ b/tools/gn/format_test_data/069.golden
@@ -0,0 +1,5 @@
+if (true) {
+ configs -= [
+ "//third_party/mini_chromium/mini_chromium/build:Wexit_time_destructors",
+ ]
+}
