Merge pull request #2541 from kbrunham-intel/fix/2540
Fix formatter non-convergence with EOL comment before else if
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc
index 7b1e66a..dce9f05 100644
--- a/verible/verilog/formatting/formatter_test.cc
+++ b/verible/verilog/formatting/formatter_test.cc
@@ -19380,6 +19380,78 @@
}
}
+// 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).
+TEST(FormatterEndToEndTest, EndElseIfWithEOLCommentConverges) {
+ static constexpr FormatterTestCase kTestCases[] = {
+ {// Comment on its own line between end and else if
+ "module m;\n"
+ " always_comb begin\n"
+ " case (state)\n"
+ " STATE_A: begin\n"
+ " if (cond_aaaa) next_state_value = STATE_B;\n"
+ " else if (cond_bbbb) begin\n"
+ " next_state_value = STATE_B;\n"
+ " end\n"
+ " // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n"
+ " else if (cond_cccc) next_state_value = STATE_C;\n"
+ " end\n"
+ " endcase\n"
+ " end\n"
+ "endmodule\n",
+ "module m;\n"
+ " always_comb begin\n"
+ " case (state)\n"
+ " STATE_A: begin\n"
+ " if (cond_aaaa) next_state_value = STATE_B;\n"
+ " else if (cond_bbbb) begin\n"
+ " next_state_value = STATE_B;\n"
+ " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n"
+ " else if (cond_cccc) next_state_value = STATE_C;\n"
+ " end\n"
+ " endcase\n"
+ " end\n"
+ "endmodule\n"},
+ {// Same construct with comment already on the end line
+ "module m;\n"
+ " always_comb begin\n"
+ " case (state)\n"
+ " STATE_A: begin\n"
+ " if (cond_aaaa) next_state_value = STATE_B;\n"
+ " else if (cond_bbbb) begin\n"
+ " next_state_value = STATE_B;\n"
+ " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n"
+ " else if (cond_cccc) next_state_value = STATE_C;\n"
+ " end\n"
+ " endcase\n"
+ " end\n"
+ "endmodule\n",
+ "module m;\n"
+ " always_comb begin\n"
+ " case (state)\n"
+ " STATE_A: begin\n"
+ " if (cond_aaaa) next_state_value = STATE_B;\n"
+ " else if (cond_bbbb) begin\n"
+ " next_state_value = STATE_B;\n"
+ " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n"
+ " else if (cond_cccc) next_state_value = STATE_C;\n"
+ " end\n"
+ " endcase\n"
+ " end\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;
+ }
+}
+
// Verify kAlign behavior for body-level param/localparam declarations
// in module and package bodies.
TEST(FormatterEndToEndTest, ParamDeclarationAlignmentBasics) {
diff --git a/verible/verilog/formatting/tree-unwrapper.cc b/verible/verilog/formatting/tree-unwrapper.cc
index 9664c98..a6601ac 100644
--- a/verible/verilog/formatting/tree-unwrapper.cc
+++ b/verible/verilog/formatting/tree-unwrapper.cc
@@ -1819,6 +1819,14 @@
}
}
+// True if any token in this leaf partition is an EOL comment.
+static bool PartitionContainsEOLComment(const TokenPartitionTree &partition) {
+ for (const auto &token : partition.Value().TokensRange()) {
+ if (token.TokenEnum() == verilog_tokentype::TK_EOL_COMMENT) return true;
+ }
+ return false;
+}
+
static void PushEndIntoElsePartition(TokenPartitionTree *partition_ptr) {
// Then combine 'end' with the following 'else' ...
// Do not flatten, so that if- and else- clauses can make formatting
@@ -1826,6 +1834,15 @@
auto &partition = *partition_ptr;
auto &if_clause_partition = partition.Children().front();
auto *end_partition = &RightmostDescendant(if_clause_partition);
+ // When 'end' carries a trailing EOL comment, 'else' must start on the next
+ // line (see token annotator: comment before else => MustWrap). Merging
+ // end+comment into the else-if header makes fit-else-expand treat the
+ // header as wider than the eventual formatted line, which wraps the
+ // else-if body on re-format and fails convergence (GitHub issue 2540).
+ if (PartitionContainsEOLComment(*end_partition)) {
+ VLOG(4) << "end has EOL comment, skip merge into else";
+ return;
+ }
auto *end_parent = verible::MergeLeafIntoNextLeaf(end_partition);
// if moving leaf results in any singleton partitions, hoist.
if (end_parent != nullptr) {
