Merge origin/master into fix/2352 Resolve the issue-regression conflict by placing the #2352 test between the end/else-if and non-ANSI port regressions, away from other open PR tests.
diff --git a/verible/verilog/formatting/formatter_issue_regression_test.cc b/verible/verilog/formatting/formatter_issue_regression_test.cc index 9490a88..2e6e3aa 100644 --- a/verible/verilog/formatting/formatter_issue_regression_test.cc +++ b/verible/verilog/formatting/formatter_issue_regression_test.cc
@@ -207,6 +207,34 @@ } } +// Regression for https://github.com/chipsalliance/verible/issues/2352: +// '/' between identifiers in a macro argument is a path separator and must +// not be spaced as a division operator (that breaks compiles). +TEST(FormatterEndToEndTest, MacroArgPathSeparatorsKeepNoSpace) { + static constexpr FormatterTestCase kTestCases[] = { + {// Original issue sample + "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n", + "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n"}, + {// Extra spaces around '/' are removed in macro args + "`PROJECT_INCLUDE(`PATH_MY_MODULE / src / config_class.sv)\n", + "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n"}, + {// Nested directories + "`INCLUDE(foo/bar/baz.svh)\n", "`INCLUDE(foo/bar/baz.svh)\n"}, + {// Path as a later argument + "`LOAD(cfg, `ROOT/hw/ip/file.sv)\n", + "`LOAD(cfg, `ROOT/hw/ip/file.sv)\n"}, + {// Division between identifiers outside macros still gets spaces + "module m;\n" + " assign x = a/b;\n" + "endmodule\n", + "module m;\n" + " assign x = a / b;\n" + "endmodule\n"}, + }; + FormatStyle style; // default column_limit (100) + RunFormatterTestCases(style, kTestCases); +} + // Regression for https://github.com/chipsalliance/verible/issues/2008 // (also https://github.com/chipsalliance/verible/issues/2474 and // https://github.com/chipsalliance/verible/issues/2063):
diff --git a/verible/verilog/formatting/token-annotator.cc b/verible/verilog/formatting/token-annotator.cc index d66042f..c427e54 100644 --- a/verible/verilog/formatting/token-annotator.cc +++ b/verible/verilog/formatting/token-annotator.cc
@@ -120,6 +120,29 @@ {NodeEnum::kConditionExpression}); // exclude } +// '/' between identifiers inside a macro argument is a filesystem path +// (e.g. `INCLUDE(`PATH/src/file.sv)), not a division operator (issue #2352). +static bool InMacroArgumentContext(const SyntaxTreeContext &context) { + return context.IsInsideFirst({NodeEnum::kMacroArgList, NodeEnum::kMacroCall, + NodeEnum::kMacroGenericItem}, + {}); +} + +static bool IsPathSeparatorSlashInMacroArg( + const PreFormatToken &left, const PreFormatToken &right, + const SyntaxTreeContext &left_context, + const SyntaxTreeContext &right_context) { + if (left.TokenEnum() != '/' && right.TokenEnum() != '/') return false; + const bool identifier_on_other_side = + (left.TokenEnum() == '/' && + right.format_token_enum == FormatTokenType::identifier) || + (right.TokenEnum() == '/' && + left.format_token_enum == FormatTokenType::identifier); + if (!identifier_on_other_side) return false; + return InMacroArgumentContext(left_context) || + InMacroArgumentContext(right_context); +} + static bool IsAnySemicolon(const PreFormatToken &ftoken) { // These are just syntactically disambiguated versions of ';'. return ftoken.TokenEnum() == ';' || @@ -228,6 +251,10 @@ // Consider assignment operators in the same class as binary operators. if (left.format_token_enum == FormatTokenType::binary_operator || right.format_token_enum == FormatTokenType::binary_operator) { + if (IsPathSeparatorSlashInMacroArg(left, right, left_context, + right_context)) { + return {0, "No space around '/' path separators in macro arguments"}; + } // Inside [], allows 0 or 1 spaces, and symmetrize. // TODO(fangism): make this behavior configurable if (right.format_token_enum == FormatTokenType::binary_operator &&
diff --git a/verible/verilog/formatting/token-annotator_test.cc b/verible/verilog/formatting/token-annotator_test.cc index c0f7e86..df27619 100644 --- a/verible/verilog/formatting/token-annotator_test.cc +++ b/verible/verilog/formatting/token-annotator_test.cc
@@ -4788,6 +4788,48 @@ {/* unspecified context */}, {1, SpacingOptions::kUndecided}, }, + // '/' between identifiers is a path separator in macro args (#2352), + // but remains a binary operator in other contexts. + { + DefaultStyle, + {verilog_tokentype::MacroIdentifier, "`PATH"}, + {'/', "/"}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {0, SpacingOptions::kUndecided}, + }, + { + DefaultStyle, + {'/', "/"}, + {verilog_tokentype::SymbolIdentifier, "src"}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {0, SpacingOptions::kUndecided}, + }, + { + DefaultStyle, + {verilog_tokentype::SymbolIdentifier, "src"}, + {'/', "/"}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {NodeEnum::kMacroArgList, NodeEnum::kMacroCall}, + {0, SpacingOptions::kUndecided}, + }, + { + DefaultStyle, + {verilog_tokentype::SymbolIdentifier, "a"}, + {'/', "/"}, + {/* expression, not a macro argument */}, + {/* expression, not a macro argument */}, + {1, SpacingOptions::kUndecided}, + }, + { + DefaultStyle, + {'/', "/"}, + {verilog_tokentype::SymbolIdentifier, "b"}, + {/* expression, not a macro argument */}, + {/* expression, not a macro argument */}, + {1, SpacingOptions::kUndecided}, + }, }; int test_index = 0; for (const auto &test_case : kTestCases) {