format: Improve suffix comment handling
Primarily this fixes the bug linked below, but also addresses some old
disabled tests which needed suffix comment wrapping. This just hadn't
been implemented before.
Test run on Chromium at
https://chromium-review.googlesource.com/c/chromium/src/+/2011169.
Bug: gn:138
Change-Id: I53e886f56a830926e958f3c43fa22d8928f2124f
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/7120
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 668fedd..3cc4218 100644
--- a/src/gn/command_format.cc
+++ b/src/gn/command_format.cc
@@ -135,6 +135,10 @@
void TrimAndPrintToken(const Token& token);
+ void PrintTrailingCommentsWrapped(const std::vector<Token>& comments);
+
+ void PrintSuffixComments(const ParseNode* node);
+
// End the current line, flushing end of line comments.
void Newline();
@@ -276,23 +280,74 @@
Print(trimmed);
}
+// Assumes that the margin is set to the indent level where the comments should
+// be aligned. This doesn't de-wrap, it only wraps. So if a suffix comment
+// causes the line to exceed 80 col it will be wrapped, but the subsequent line
+// would fit on the then-broken line it will not be merged with it. This is
+// partly because it's difficult to implement at this level, but also because
+// it can break hand-authored line breaks where they're starting a new paragraph
+// or statement.
+void Printer::PrintTrailingCommentsWrapped(const std::vector<Token>& comments) {
+ bool have_empty_line = true;
+ auto start_next_line = [this, &have_empty_line]() {
+ Trim();
+ Print("\n");
+ PrintMargin();
+ have_empty_line = true;
+ };
+ for (const auto& c : comments) {
+ if (!have_empty_line) {
+ start_next_line();
+ }
+
+ std::string trimmed;
+ TrimWhitespaceASCII(std::string(c.value()), base::TRIM_ALL, &trimmed);
+
+ if (margin() + trimmed.size() <= kMaximumWidth) {
+ Print(trimmed);
+ have_empty_line = false;
+ } else {
+ bool continuation = false;
+ std::vector<std::string> split_on_spaces = base::SplitString(
+ c.value(), " ", base::WhitespaceHandling::TRIM_WHITESPACE,
+ base::SplitResult::SPLIT_WANT_NONEMPTY);
+ for (size_t j = 0; j < split_on_spaces.size(); ++j) {
+ if (have_empty_line && continuation) {
+ Print("# ");
+ }
+ Print(split_on_spaces[j]) ;
+ Print(" ");
+ if (split_on_spaces[j] != "#") {
+ have_empty_line = false;
+ }
+ if (!have_empty_line &&
+ (j < split_on_spaces.size() - 1 &&
+ CurrentColumn() + split_on_spaces[j + 1].size() > kMaximumWidth)) {
+ start_next_line();
+ continuation = true;
+ }
+ }
+ }
+ }
+}
+
+// Used during penalty evaluation, similar to Newline().
+void Printer::PrintSuffixComments(const ParseNode* node) {
+ if (node->comments() && !node->comments()->suffix().empty()) {
+ Print(" ");
+ stack_.push_back(IndentState(CurrentColumn(), false, false));
+ PrintTrailingCommentsWrapped(node->comments()->suffix());
+ stack_.pop_back();
+ }
+}
+
void Printer::Newline() {
if (!comments_.empty()) {
Print(" ");
// Save the margin, and temporarily set it to where the first comment
- // starts so that multiple suffix comments are vertically aligned. This
- // will need to be fancier once we enforce 80 col.
+ // starts so that multiple suffix comments are vertically aligned.
stack_.push_back(IndentState(CurrentColumn(), false, false));
- int i = 0;
- for (const auto& c : comments_) {
- if (i > 0) {
- Trim();
- Print("\n");
- PrintMargin();
- }
- TrimAndPrintToken(c);
- ++i;
- }
+ PrintTrailingCommentsWrapped(comments_);
stack_.pop_back();
comments_.clear();
}
@@ -519,8 +574,11 @@
bool Printer::ExceedsMaximumWidth(const std::string& output) {
for (const auto& line : base::SplitString(output, "\n", base::KEEP_WHITESPACE,
base::SPLIT_WANT_ALL)) {
- if (line.size() > kMaximumWidth)
+ std::string_view trimmed =
+ TrimString(line, " ", base::TrimPositions::TRIM_TRAILING);
+ if (trimmed.size() > kMaximumWidth) {
return true;
+ }
}
return false;
}
@@ -639,6 +697,8 @@
int penalty_current_line =
sub1.Expr(binop->right(), prec_right, std::string());
sub1.Print(suffix);
+ sub1.PrintSuffixComments(root);
+ sub1.PrintSuffixComments(binop->right());
penalty_current_line += AssessPenalty(sub1.String());
if (!is_assignment && left_is_multiline) {
// In e.g. xxx + yyy, if xxx is already multiline, then we want a penalty
@@ -654,6 +714,8 @@
int penalty_next_line =
sub2.Expr(binop->right(), prec_right, std::string());
sub2.Print(suffix);
+ sub2.PrintSuffixComments(root);
+ sub2.PrintSuffixComments(binop->right());
penalty_next_line += AssessPenalty(sub2.String());
// Force a list on the RHS that would normally be a single line into
@@ -669,6 +731,8 @@
sub3.stack_.push_back(IndentState(start_column, false, false));
sub3.Sequence(kSequenceStyleList, rhs_list->contents(), rhs_list->End(),
true);
+ sub3.PrintSuffixComments(root);
+ sub3.PrintSuffixComments(binop->right());
sub3.stack_.pop_back();
penalty_multiline_rhs_list = AssessPenalty(sub3.String());
tried_rhs_multiline = true;
@@ -1026,10 +1090,13 @@
if (end && end->comments() && !end->comments()->before().empty())
return true;
- // If there's before line comments, make sure we have a place to put them.
+ // If there's before or suffix line comments, make sure we have a place to put
+ // them.
for (const auto& i : list) {
- if (i->comments() && !i->comments()->before().empty())
+ if (i->comments() && (!i->comments()->before().empty() ||
+ !i->comments()->suffix().empty())) {
return true;
+ }
}
// When a scope is used as a list entry, it's too complicated to go one a
diff --git a/src/gn/command_format_unittest.cc b/src/gn/command_format_unittest.cc
index f334b70..503e6cf 100644
--- a/src/gn/command_format_unittest.cc
+++ b/src/gn/command_format_unittest.cc
@@ -83,11 +83,11 @@
FORMAT_TEST(037)
FORMAT_TEST(038)
FORMAT_TEST(039)
-// TODO(scottmg): Bad break, exceeding 80 col: FORMAT_TEST(040)
+FORMAT_TEST(040)
FORMAT_TEST(041)
FORMAT_TEST(042)
FORMAT_TEST(043)
-// TODO(scottmg): Dewrapped caused exceeding 80 col: FORMAT_TEST(044)
+FORMAT_TEST(044)
FORMAT_TEST(045)
FORMAT_TEST(046)
FORMAT_TEST(047)
@@ -121,3 +121,6 @@
FORMAT_TEST(074)
FORMAT_TEST(075)
FORMAT_TEST(076)
+FORMAT_TEST(077)
+FORMAT_TEST(078)
+FORMAT_TEST(079)
diff --git a/src/gn/format_test_data/040.gn b/src/gn/format_test_data/040.gn
index 1f4754e..63e5b07 100644
--- a/src/gn/format_test_data/040.gn
+++ b/src/gn/format_test_data/040.gn
@@ -1,4 +1,4 @@
-# Dewrapping shouldn't cause 80 col to be exceed.
+# Dewrapping shouldn't cause 80 col to be exceeded.
# 80 ---------------------------------------------------------------------------
if (true) {
if (is_win) {
diff --git a/src/gn/format_test_data/040.golden b/src/gn/format_test_data/040.golden
new file mode 100644
index 0000000..89fdacf
--- /dev/null
+++ b/src/gn/format_test_data/040.golden
@@ -0,0 +1,10 @@
+# Dewrapping shouldn't cause 80 col to be exceeded.
+# 80 ---------------------------------------------------------------------------
+if (true) {
+ if (is_win) {
+ cflags = [
+ "/wd4267", # TODO(jschuh): crbug.com/167187 fix size_t to int
+ # truncations.
+ ]
+ }
+}
diff --git a/src/gn/format_test_data/044.gn b/src/gn/format_test_data/044.gn
index fe09617..030c5dd 100644
--- a/src/gn/format_test_data/044.gn
+++ b/src/gn/format_test_data/044.gn
@@ -1,3 +1,4 @@
+# 80 ---------------------------------------------------------------------------
config("compiler") {
if (is_win) {
if (is_component_build) {
diff --git a/src/gn/format_test_data/077.gn b/src/gn/format_test_data/077.gn
new file mode 100644
index 0000000..665d420
--- /dev/null
+++ b/src/gn/format_test_data/077.gn
@@ -0,0 +1,6 @@
+# Regression test for https://crbug.com/gn/138. 80 col -------------------------
+foo("bar") {
+ blah = [
+ "$target_gen_dir/run-lit", # Non-existing, so that ninja runs it each time.
+ ]
+}
diff --git a/src/gn/format_test_data/077.golden b/src/gn/format_test_data/077.golden
new file mode 100644
index 0000000..665d420
--- /dev/null
+++ b/src/gn/format_test_data/077.golden
@@ -0,0 +1,6 @@
+# Regression test for https://crbug.com/gn/138. 80 col -------------------------
+foo("bar") {
+ blah = [
+ "$target_gen_dir/run-lit", # Non-existing, so that ninja runs it each time.
+ ]
+}
diff --git a/src/gn/format_test_data/078.gn b/src/gn/format_test_data/078.gn
new file mode 100644
index 0000000..9b1369a
--- /dev/null
+++ b/src/gn/format_test_data/078.gn
@@ -0,0 +1,15 @@
+# 80 ---------------------------------------------------------------------------
+# Long suffix comments, and including trailing spaces.
+config("compiler") {
+ if (is_win) {
+ if (is_component_build) {
+ cflags += [
+ "/EHsc", # These are some very long suffix comment words aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ # bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+ # cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
+ # dddddddddddddddd ddddddddddd dddddddd ddddddddddddd ddddddddddddddd
+ ]
+ }
+ }
+}
+
diff --git a/src/gn/format_test_data/078.golden b/src/gn/format_test_data/078.golden
new file mode 100644
index 0000000..ea96580
--- /dev/null
+++ b/src/gn/format_test_data/078.golden
@@ -0,0 +1,16 @@
+# 80 ---------------------------------------------------------------------------
+# Long suffix comments, and including trailing spaces.
+config("compiler") {
+ if (is_win) {
+ if (is_component_build) {
+ cflags += [
+ "/EHsc", # These are some very long suffix comment words
+ # aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ # bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+ # cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
+ # dddddddddddddddd ddddddddddd dddddddd ddddddddddddd
+ # ddddddddddddddd
+ ]
+ }
+ }
+}
diff --git a/src/gn/format_test_data/079.gn b/src/gn/format_test_data/079.gn
new file mode 100644
index 0000000..9218757
--- /dev/null
+++ b/src/gn/format_test_data/079.gn
@@ -0,0 +1,14 @@
+# 80 ---------------------------------------------------------------------------
+# Somewhat more intelligent suffix comments.
+
+core_generated_interface_idl_files = generated_webcore_testing_idl_files # interfaces
+
+core_generated_interface_idl_files = generated_webcore_testing_idl_files_and_some_more_longer # stuff
+
+if (true) {
+ if (true) {
+ if (true) {
+ dllname = "{{output_dir}}/{{target_output_name}}{{output_extension}}" # e.g. foo.dll
+ }
+ }
+}
diff --git a/src/gn/format_test_data/079.golden b/src/gn/format_test_data/079.golden
new file mode 100644
index 0000000..c4617b5
--- /dev/null
+++ b/src/gn/format_test_data/079.golden
@@ -0,0 +1,18 @@
+# 80 ---------------------------------------------------------------------------
+# Somewhat more intelligent suffix comments.
+
+core_generated_interface_idl_files =
+ generated_webcore_testing_idl_files # interfaces
+
+core_generated_interface_idl_files =
+ generated_webcore_testing_idl_files_and_some_more_longer # stuff
+
+if (true) {
+ if (true) {
+ if (true) {
+ dllname =
+ "{{output_dir}}/{{target_output_name}}{{output_extension}}" # e.g.
+ # foo.dll
+ }
+ }
+}
