format: Fix formatting not taking into account suffixes
In subprinters (evaluated for penalty values), suffixes from higher up
the expression tree where not being included, resulting in incorrect
penalties. In some cases, this caused the formatter to believe that all
options were going to exceed 80 col, and so it failed badly by putting
[way] too much on a single line.
https://chromium-review.googlesource.com/c/chromium/src/+/2124534 is a
test run on all .gn/.gni files in Chromium. Only the first two files in
the CL are related to this change, and seem like improvements or
approximately as good.
Bug: gn:156
Change-Id: I017ef6bc2918c1c8fdbd971c1041f5069b4c26c7
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/7880
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
diff --git a/src/gn/command_format.cc b/src/gn/command_format.cc
index 3253fde..d475893 100644
--- a/src/gn/command_format.cc
+++ b/src/gn/command_format.cc
@@ -183,10 +183,11 @@
// will be set so it can be closed at the end of the expression.
void AddParen(int prec, int outer_prec, bool* parenthesized);
- // Print the expression to the output buffer. Returns the type of element
- // added to the output. The value of outer_prec gives the precedence of the
- // operator outside this Expr. If that operator binds tighter than root's,
- // Expr must introduce parentheses.
+ // Print the expression given by |root| to the output buffer and appends
+ // |suffix| to that output. Returns a penalty that represents the cost of
+ // adding that output to the buffer (where higher is worse). The value of
+ // outer_prec gives the precedence of the operator outside this Expr. If that
+ // operator binds tighter than root's, Expr() must introduce parentheses.
int Expr(const ParseNode* root, int outer_prec, const std::string& suffix);
// Generic penalties for exceeding maximum width, adding more lines, etc.
@@ -740,6 +741,9 @@
AddParen(prec_left, outer_prec, &parenthesized);
}
+ if (parenthesized)
+ at_end = ")" + at_end;
+
int start_line = CurrentLine();
int start_column = CurrentColumn();
bool is_assignment = binop->op().value() == "=" ||
@@ -785,9 +789,7 @@
Printer sub1;
InitializeSub(&sub1);
sub1.Print(" ");
- int penalty_current_line =
- sub1.Expr(binop->right(), prec_right, std::string());
- sub1.Print(suffix);
+ int penalty_current_line = sub1.Expr(binop->right(), prec_right, at_end);
sub1.PrintSuffixComments(root);
sub1.FlushComments();
penalty_current_line += AssessPenalty(sub1.String());
@@ -802,9 +804,7 @@
Printer sub2;
InitializeSub(&sub2);
sub2.Newline();
- int penalty_next_line =
- sub2.Expr(binop->right(), prec_right, std::string());
- sub2.Print(suffix);
+ int penalty_next_line = sub2.Expr(binop->right(), prec_right, at_end);
sub2.PrintSuffixComments(root);
sub2.FlushComments();
penalty_next_line += AssessPenalty(sub2.String());
@@ -841,7 +841,8 @@
if (penalty_current_line < penalty_next_line || exceeds_maximum_all_ways) {
Print(" ");
- Expr(binop->right(), prec_right, std::string());
+ Expr(binop->right(), prec_right, at_end);
+ at_end = "";
} else if (tried_rhs_multiline &&
penalty_multiline_rhs_list < penalty_next_line) {
// Force a multiline list on the right.
@@ -854,7 +855,8 @@
Newline();
penalty += std::abs(CurrentColumn() - start_column) *
kPenaltyHorizontalSeparation;
- Expr(binop->right(), prec_right, std::string());
+ Expr(binop->right(), prec_right, at_end);
+ at_end = "";
}
stack_.pop_back();
penalty += (CurrentLine() - start_line) * GetPenaltyForLineBreak();
@@ -863,6 +865,7 @@
false);
} else if (const ConditionNode* condition = root->AsConditionNode()) {
Print("if (");
+ CHECK(at_end.empty());
Expr(condition->condition(), kPrecedenceLowest, ") {");
Sequence(kSequenceStyleBracedBlockAlreadyOpen,
condition->if_true()->statements(), condition->if_true()->End(),
@@ -901,9 +904,6 @@
CHECK(false) << "Unhandled case in Expr.";
}
- if (parenthesized)
- Print(")");
-
// Defer any end of line comment until we reach the newline.
if (root->comments() && !root->comments()->suffix().empty()) {
std::copy(root->comments()->suffix().begin(),
diff --git a/src/gn/command_format_unittest.cc b/src/gn/command_format_unittest.cc
index 5f814b9..4dea55e 100644
--- a/src/gn/command_format_unittest.cc
+++ b/src/gn/command_format_unittest.cc
@@ -126,3 +126,4 @@
FORMAT_TEST(079)
FORMAT_TEST(080)
FORMAT_TEST(081)
+FORMAT_TEST(082)
diff --git a/src/gn/format_test_data/082.gn b/src/gn/format_test_data/082.gn
new file mode 100644
index 0000000..2ced597
--- /dev/null
+++ b/src/gn/format_test_data/082.gn
@@ -0,0 +1,34 @@
+# https://crbug.com/gn/156 -----------------------------------------------------
+if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+ (bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ||
+ cccccccccccccccccccccc)) {
+}
+
+if (!is_nacl && !use_libfzzzzuzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "winnnnn")) {
+ print("hi")
+}
+
+foo("bar") {
+ if (foo) {
+ if (!is_nacl && !use_libfuzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "win")) {
+ cflags += [ "-Wunreachable-code" ]
+ }
+ }
+}
+
+foo("bar") {
+ if (foo) {
+ if (!is_nacl && !use_libfuzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "win")) {
+ cflags += [ "-Wunreachable-code" ]
+ }
+ }
+}
diff --git a/src/gn/format_test_data/082.golden b/src/gn/format_test_data/082.golden
new file mode 100644
index 0000000..2ced597
--- /dev/null
+++ b/src/gn/format_test_data/082.golden
@@ -0,0 +1,34 @@
+# https://crbug.com/gn/156 -----------------------------------------------------
+if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+ (bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ||
+ cccccccccccccccccccccc)) {
+}
+
+if (!is_nacl && !use_libfzzzzuzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "winnnnn")) {
+ print("hi")
+}
+
+foo("bar") {
+ if (foo) {
+ if (!is_nacl && !use_libfuzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "win")) {
+ cflags += [ "-Wunreachable-code" ]
+ }
+ }
+}
+
+foo("bar") {
+ if (foo) {
+ if (!is_nacl && !use_libfuzzer &&
+ (target_os == "android" || target_os == "chromeos" ||
+ target_os == "fuchsia" || target_os == "linux" ||
+ target_os == "win")) {
+ cflags += [ "-Wunreachable-code" ]
+ }
+ }
+}
