Fix formatter non-convergence for continuation EOL comments Count only the width after the last newline in preserved leading spaces when placing continuation comments after wrapped lines, so re-formatting keeps a stable column (issue #2542).
diff --git a/verible/verilog/formatting/formatter.cc b/verible/verilog/formatting/formatter.cc index 9893b60..80ec58d 100644 --- a/verible/verilog/formatting/formatter.cc +++ b/verible/verilog/formatting/formatter.cc
@@ -743,31 +743,19 @@ return column; } - static void AdjustColumnUsingTokenSpacing( - const verible::FormattedToken &token, int *column) { - switch (token.before.action) { - case verible::SpacingDecision::kPreserve: { - if (token.before.preserved_space_start != - verible::string_view_null_iterator()) { - *column += token.OriginalLeadingSpaces().length(); - } else { - *column += token.before.spaces; - } - break; - } - case verible::SpacingDecision::kWrap: - *column = 0; - ABSL_FALLTHROUGH_INTENDED; - case verible::SpacingDecision::kAlign: - case verible::SpacingDecision::kAppend: - *column += token.before.spaces; - break; - } - } - 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(); + const auto &tokens = line.Tokens(); + const auto &front = tokens.front(); if (front.before.action != verible::SpacingDecision::kPreserve) { column += line.IndentationSpaces(); @@ -777,12 +765,39 @@ } column += front.token->text().length(); - for (const auto &ftoken : verible::make_range(line.Tokens().begin() + 1, - line.Tokens().end() - 1)) { - AdjustColumnUsingTokenSpacing(ftoken, &column); - column += ftoken.token->text().length(); + for (size_t i = 1; i < tokens.size(); ++i) { + const auto &ftoken = tokens[i]; + switch (ftoken.before.action) { + case verible::SpacingDecision::kPreserve: { + if (ftoken.before.preserved_space_start != + verible::string_view_null_iterator()) { + const std::string_view leading = ftoken.OriginalLeadingSpaces(); + const auto last_nl = leading.find_last_of('\n'); + if (last_nl == std::string_view::npos) { + column += leading.length(); + } else { + column = static_cast<int>(leading.length() - last_nl - 1); + } + } else { + column += ftoken.before.spaces; + } + break; + } + case verible::SpacingDecision::kWrap: + // Newline then only the wrap indent (same as FormattedToken emit). + column = ftoken.before.spaces; + break; + case verible::SpacingDecision::kAlign: + case verible::SpacingDecision::kAppend: + column += ftoken.before.spaces; + break; + } + // 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 e885d74..bbfa1bc 100644 --- a/verible/verilog/formatting/formatter_test.cc +++ b/verible/verilog/formatting/formatter_test.cc
@@ -19216,6 +19216,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; + } +} + } // namespace } // namespace formatter } // namespace verilog