Merge pull request #2543 from kbrunham-intel/fix/2542
Fix formatter non-convergence for continuation EOL comments
diff --git a/verible/verilog/formatting/BUILD b/verible/verilog/formatting/BUILD
index 6150594..6431226 100644
--- a/verible/verilog/formatting/BUILD
+++ b/verible/verilog/formatting/BUILD
@@ -163,7 +163,6 @@
"//verible/common/util:expandable-tree-view",
"//verible/common/util:interval",
"//verible/common/util:interval-set",
- "//verible/common/util:iterator-range",
"//verible/common/util:logging",
"//verible/common/util:spacer",
"//verible/common/util:tree-operations",
@@ -175,7 +174,6 @@
"//verible/verilog/analysis:verilog-equivalence",
"//verible/verilog/parser:verilog-token-enum",
"//verible/verilog/preprocessor:verilog-preprocess",
- "@abseil-cpp//absl/base:core_headers",
"@abseil-cpp//absl/log:die_if_null",
"@abseil-cpp//absl/status",
"@abseil-cpp//absl/status:statusor",
diff --git a/verible/verilog/formatting/formatter.cc b/verible/verilog/formatting/formatter.cc
index 9893b60..9bd3dd2 100644
--- a/verible/verilog/formatting/formatter.cc
+++ b/verible/verilog/formatting/formatter.cc
@@ -26,7 +26,6 @@
#include <string_view>
#include <vector>
-#include "absl/base/attributes.h"
#include "absl/log/die_if_null.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
@@ -50,7 +49,6 @@
#include "verible/common/util/expandable-tree-view.h"
#include "verible/common/util/interval-set.h"
#include "verible/common/util/interval.h"
-#include "verible/common/util/iterator-range.h"
#include "verible/common/util/logging.h"
#include "verible/common/util/spacer.h"
#include "verible/common/util/tree-operations.h"
@@ -749,15 +747,24 @@
case verible::SpacingDecision::kPreserve: {
if (token.before.preserved_space_start !=
verible::string_view_null_iterator()) {
- *column += token.OriginalLeadingSpaces().length();
+ const std::string_view leading = token.OriginalLeadingSpaces();
+ const auto last_nl = leading.find_last_of('\n');
+ if (last_nl == std::string_view::npos) {
+ *column += leading.length();
+ } else {
+ // Reset column after the last newline, same as FormattedToken
+ // emit (GitHub issue 2542).
+ *column = static_cast<int>(leading.length() - last_nl - 1);
+ }
} else {
*column += token.before.spaces;
}
break;
}
case verible::SpacingDecision::kWrap:
- *column = 0;
- ABSL_FALLTHROUGH_INTENDED;
+ // Newline then only the wrap indent (same as FormattedToken emit).
+ *column = token.before.spaces;
+ break;
case verible::SpacingDecision::kAlign:
case verible::SpacingDecision::kAppend:
*column += token.before.spaces;
@@ -766,6 +773,15 @@
}
static int CalculateEolCommentColumn(const verible::FormattedExcerpt &line) {
+ // Compute the starting column of the trailing EOL comment the same way
+ // FormattedExcerpt::FormattedText emits spaces, including:
+ // * wrap indents (SpacingDecision::kWrap), and
+ // * preserved leading whitespace that may contain newlines (common when
+ // an original line break is kept). Counting those newlines as width
+ // made continuation comments land on the wrong column and fail to
+ // converge on re-format (GitHub issue 2542).
+ if (line.Tokens().empty()) return 0;
+
int column = 0;
const auto &front = line.Tokens().front();
@@ -777,12 +793,16 @@
}
column += front.token->text().length();
- for (const auto &ftoken : verible::make_range(line.Tokens().begin() + 1,
- line.Tokens().end() - 1)) {
+ const auto &tokens = line.Tokens();
+ for (size_t i = 1; i < tokens.size(); ++i) {
+ const auto &ftoken = tokens[i];
AdjustColumnUsingTokenSpacing(ftoken, &column);
- column += ftoken.token->text().length();
+ // Do not add the last token's length: that is the EOL comment whose
+ // starting column we want.
+ if (i + 1 < tokens.size()) {
+ column += ftoken.token->text().length();
+ }
}
- AdjustColumnUsingTokenSpacing(line.Tokens().back(), &column);
CHECK_GE(column, 0);
return column;
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc
index dce9f05..61950a0 100644
--- a/verible/verilog/formatting/formatter_test.cc
+++ b/verible/verilog/formatting/formatter_test.cc
@@ -19380,6 +19380,50 @@
}
}
+// Regression for https://github.com/chipsalliance/verible/issues/2542:
+// Continuation EOL comments after a wrapped assign must keep a stable column
+// across re-format (convergence).
+TEST(FormatterEndToEndTest, ContinuationCommentAfterWrappedAssignConverges) {
+ static constexpr FormatterTestCase kTestCases[] = {
+ {// Comments originally column-aligned after a wrapped assign
+ "module m;\n"
+ " assign status_ur = !(status_sc || status_ca ||\n"
+ " status_crs); // Completions with a Reserved Completion\n"
+ " // Status value are treated as UR\n"
+ "endmodule\n",
+ "module m;\n"
+ " assign status_ur =\n"
+ " !(status_sc || status_ca || status_crs); // Completions with a "
+ "Reserved Completion\n"
+ " // Status value are "
+ "treated as UR\n"
+ "endmodule\n"},
+ {// Previously mis-aligned continuation is not treated as a continuation
+ // (column delta > 1) and must still converge
+ "module m;\n"
+ " assign status_ur = !(status_sc || status_ca ||\n"
+ " status_crs); // Completions with a Reserved Completion\n"
+ " "
+ "// Status value are treated as UR\n"
+ "endmodule\n",
+ "module m;\n"
+ " assign status_ur =\n"
+ " !(status_sc || status_ca || status_crs); // Completions with a "
+ "Reserved Completion\n"
+ " // Status value are treated as UR\n"
+ "endmodule\n"},
+ };
+ FormatStyle style; // default column_limit (100)
+ for (const auto &test_case : kTestCases) {
+ VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>";
+ std::ostringstream stream;
+ const auto status =
+ FormatVerilog(test_case.input, "<filename>", style, stream);
+ EXPECT_OK(status) << status.message();
+ EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input;
+ }
+}
+
// Regression for https://github.com/chipsalliance/verible/issues/2540:
// Trailing EOL comment after `end` before `else if` must not change whether
// the else-if assignment stays on one line across re-format (convergence).
