Merge pull request #2548 from kbrunham-intel/fix/2547
Fix formatter non-convergence for multi-line macro sums
diff --git a/verible/verilog/analysis/verilog-equivalence.cc b/verible/verilog/analysis/verilog-equivalence.cc
index 68c8c98..d5c12bea 100644
--- a/verible/verilog/analysis/verilog-equivalence.cc
+++ b/verible/verilog/analysis/verilog-equivalence.cc
@@ -89,11 +89,17 @@
return IsUnlexed(verilog_tokentype(token.token_enum()));
}
-// MacroIdentifier vs MacroIdItem depends only on whether the macro ends the
-// line (see POST_MACRO_ID in verilog.lex). Spelling-equal macros are
-// format-equivalent across that reclassification.
-static bool AreSpellingEqualLineEndingMacros(const TokenInfo &left,
- const TokenInfo &right) {
+// True when left/right differ only because surrounding whitespace changed
+// token classification (e.g. MacroCallCloseToEndLine vs ')', or
+// MacroIdentifier vs MacroIdItem with unchanged spelling).
+static bool TokensAreWhitespaceDependentFormatEquivalent(
+ const TokenInfo &left, const TokenInfo &right) {
+ if ((left.token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
+ right.text() == ")") ||
+ (right.token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
+ left.text() == ")")) {
+ return true;
+ }
const auto is_line_ending_macro = [](int token_enum) {
return token_enum == verilog_tokentype::MacroIdentifier ||
token_enum == verilog_tokentype::MacroIdItem;
@@ -184,14 +190,8 @@
// Some token enums differ only by surrounding whitespace (e.g. whether a
// macro or ')' ends a line). Treat those pairs as matching enums when the
// spelling is unchanged so FormatEquivalent tolerates re-wrapping.
- const bool whitespace_dependent_macro_enum_match =
- ((l->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
- r->text() == ")") ||
- (r->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
- l->text() == ")") ||
- AreSpellingEqualLineEndingMacros(*l, *r));
if (l->token_enum() != r->token_enum() &&
- !whitespace_dependent_macro_enum_match) {
+ !TokensAreWhitespaceDependentFormatEquivalent(*l, *r)) {
if (errstream != nullptr) {
*errstream << "Mismatched token enums. got: ";
token_printer(*l, *errstream);
@@ -270,15 +270,7 @@
return IsWhitespace(verilog_tokentype(t.token_enum()));
},
[=](const TokenInfo &l, const TokenInfo &r) {
- // MacroCallCloseToEndLine should be considered equivalent to ')', as
- // they are whitespace dependant
- if (((r.token_enum() == verilog_tokentype::MacroCallCloseToEndLine) &&
- (l.text() == ")")) ||
- ((l.token_enum() == verilog_tokentype::MacroCallCloseToEndLine) &&
- (r.text() == ")"))) {
- return true;
- }
- if (AreSpellingEqualLineEndingMacros(l, r)) {
+ if (TokensAreWhitespaceDependentFormatEquivalent(l, r)) {
return true;
}
return l.EquivalentWithoutLocation(r);
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc
index 13a9fb4..16e0572 100644
--- a/verible/verilog/formatting/formatter_test.cc
+++ b/verible/verilog/formatting/formatter_test.cc
@@ -19456,6 +19456,50 @@
}
}
+// Regression for https://github.com/chipsalliance/verible/issues/2547:
+// A localparam initialized to a sum of long macros must converge: infix `+`
+// stays with the following operand so re-format does not oscillate between
+// `+\n`MACRO` and `+ `MACRO`.
+TEST(FormatterEndToEndTest, LongMacroSumLocalparamConverges) {
+ static constexpr FormatterTestCase kTestCases[] = {
+ {"module m;\n"
+ " localparam N =\n"
+ " `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGOUT\n"
+ " ;\n"
+ "endmodule\n",
+ "module m;\n"
+ " localparam N =\n"
+ " `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGOUT;\n"
+ "endmodule\n"},
+ // Already in pass-1 form must stay stable.
+ {"module m;\n"
+ " localparam N =\n"
+ " `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGOUT;\n"
+ "endmodule\n",
+ "module m;\n"
+ " localparam N =\n"
+ " `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+ " + `MACRO_GEN3_SCRAMBLE_REGOUT;\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/2544:
// Wrapping a $bits(...)'(...) cast may leave `MACRO at EOL, reclassifying
// MacroIdentifier as MacroIdItem. FormatEquivalent must accept that, and
diff --git a/verible/verilog/formatting/tree-unwrapper.cc b/verible/verilog/formatting/tree-unwrapper.cc
index 2286f8c..09fa8c0 100644
--- a/verible/verilog/formatting/tree-unwrapper.cc
+++ b/verible/verilog/formatting/tree-unwrapper.cc
@@ -1641,6 +1641,53 @@
}
}
+// True when a leaf partition contains only a binary/infix operator (and
+// optional comments/attributes). Used to normalize wrapping of expressions
+// like `A + B + C` so operators always stay with the following operand.
+static bool PartitionIsInfixOperatorOnly(const TokenPartitionTree &partition) {
+ if (!is_leaf(partition)) return false;
+ const auto tokens = partition.Value().TokensRange();
+ if (tokens.empty()) return false;
+
+ const verible::PreFormatToken *op = nullptr;
+ for (const auto &token : tokens) {
+ switch (token.TokenEnum()) {
+ case verilog_tokentype::TK_COMMENT_BLOCK:
+ case verilog_tokentype::TK_EOL_COMMENT:
+ case verilog_tokentype::TK_ATTRIBUTE:
+ break;
+ default:
+ if (GetFormatTokenType(static_cast<verilog_tokentype>(
+ token.TokenEnum())) != FormatTokenType::binary_operator ||
+ op != nullptr) {
+ return false;
+ }
+ op = &token;
+ break;
+ }
+ }
+ return op != nullptr;
+}
+
+// Always attach infix-operator-only partitions to the following operand.
+// Attachment based on original newlines is unstable for macro sums:
+// `A\n+\n`B vs `A\n+ `B produce different partition shapes and oscillate
+// under re-format (GitHub issue 2547).
+static void AttachInfixOperatorsToFollowingOperands(
+ TokenPartitionTree *partition) {
+ // Iterate by index; merges invalidate sibling pointers.
+ for (int i = 0; i < static_cast<int>(partition->Children().size()); ++i) {
+ auto &child = partition->Children()[i];
+ if (!PartitionIsInfixOperatorOnly(child)) continue;
+ if (NextLeaf(child) == nullptr) continue;
+ VLOG(4) << "Attaching infix operator partition to following operand:\n"
+ << child;
+ verible::MergeLeafIntoNextLeaf(&child);
+ // Children shifted; re-check current index.
+ --i;
+ }
+}
+
static void AttachTrailingSemicolonToPreviousPartition(
TokenPartitionTree *partition) {
// TODO(mglb): Replace this function with
@@ -3137,6 +3184,7 @@
case NodeEnum::kParamDeclaration: {
AttachTrailingSemicolonToPreviousPartition(&partition);
AttachOpeningBraceToDeclarationsAssignmentOperator(&partition);
+ AttachInfixOperatorsToFollowingOperands(&partition);
break;
}
