Merge origin/master into fix/886 Resolve the issue-regression conflict by placing the #886 tests before the end/else-if regression, away from other open PR tests.
diff --git a/.bazelrc b/.bazelrc index 301b564..dc2e33d 100644 --- a/.bazelrc +++ b/.bazelrc
@@ -1,6 +1,8 @@ # bazel < 7 needs explicit enabling of bzlmod dependencies. build --enable_bzlmod +test --test_output=errors + # Enable support for absl types like string_view in gtest. build --define="absl=1"
diff --git a/.github/bin/check-potential-problems.sh b/.github/bin/check-potential-problems.sh index 1af0f5f..c7d9f97 100755 --- a/.github/bin/check-potential-problems.sh +++ b/.github/bin/check-potential-problems.sh
@@ -108,11 +108,9 @@ EXIT_CODE=1 fi -# Need to skip this until https://github.com/chipsalliance/verible/issues/2435 -# resolved. -#if [ -e .bazelversion ]; then -# echo "Don't use .bazelversion. It is a poorly implemented bazel feature that does not support semantic versioning. Instead, make the repo work with all currently active bazel versions." -# EXIT_CODE=1 -#fi +if [ -e .bazelversion ]; then + echo "Don't use .bazelversion. It is a poorly implemented bazel feature that does not support semantic versioning. Instead, make the repo work with all currently active bazel versions." + EXIT_CODE=1 +fi exit "${EXIT_CODE}"
diff --git a/.github/bin/run-build-cleaner.sh b/.github/bin/run-build-cleaner.sh index d5b03f7..e372646 100755 --- a/.github/bin/run-build-cleaner.sh +++ b/.github/bin/run-build-cleaner.sh
@@ -17,19 +17,25 @@ set -e BANT=$($(dirname $0)/get-bant-path.sh) +BAZEL=bazel # Run build so that we have all dependencies downloaded and genrules # materialized. -bazel build -k --remote_download_outputs=all ... +for f in abseil-cpp nlohmann_json protobuf re2 rules_flex zlib googletest ; do + "${BAZEL}" fetch --repo "@$f" > /dev/null 2>&1 +done -if "${BANT}" -q dwyu ... ; then +"${BAZEL}" build -k --remote_download_outputs=all \ + $(${BANT} genrule-outputs ... -c2) > /dev/null 2>&1 + +if "${BANT}" dwyu $@; then echo "Dependencies ok." >&2 else cat >&2 <<EOF Build dependency issues found, the following one-liner will fix it. Amend PR. -source <(.github/bin/run-build-cleaner.sh) +source <(.github/bin/run-build-cleaner.sh $@) EOF exit 1 fi
diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index 802a7a3..31a7c21 100644 --- a/.github/workflows/verible-ci.yml +++ b/.github/workflows/verible-ci.yml
@@ -51,7 +51,7 @@ echo "--- check formatting ---" CLANG_FORMAT=clang-format-19 ./.github/bin/run-format.sh --show-diff echo "--- check build dependencies ---" - ./.github/bin/run-build-cleaner.sh + ./.github/bin/run-build-cleaner.sh ... echo "--- check potential problems ---" ./.github/bin/check-potential-problems.sh
diff --git a/verible/common/formatting/align.cc b/verible/common/formatting/align.cc index 74bc432..9f77014 100644 --- a/verible/common/formatting/align.cc +++ b/verible/common/formatting/align.cc
@@ -1001,6 +1001,20 @@ auto &line = node.Value(); auto ftokens = line.TokensRange(); + // Leading non-tree tokens before a forced wrap must stay on their own + // line. Putting them in a kInline prolog cell would glue e.g. `//\` onto + // the following `input` (GitHub issue 2539). Preserve original spacing + // for the whole row instead. Master's scanner already omits those + // leading tokens from column bids; this keeps ApplyAlignment from + // still inlining them. + if (align_actions.front().ftoken != ftokens.begin() && + align_actions.front().ftoken->before.break_decision == + SpacingOptions::kMustWrap) { + FormatUsingOriginalSpacing(TokenPartitionRange(*row, std::next(*row))); + ++row; + continue; + } + line.SetPartitionPolicy(PartitionPolicyEnum::kAlreadyFormatted); verible::TokenPartitionTree *current_cell = nullptr;
diff --git a/verible/verilog/formatting/align.cc b/verible/verilog/formatting/align.cc index 6640adb..24696c7 100644 --- a/verible/verilog/formatting/align.cc +++ b/verible/verilog/formatting/align.cc
@@ -147,6 +147,32 @@ b == AlignmentGroupBoundary::kBlankLinesAndSeparatorComments; } +// True when non-tree tokens (e.g. // comments, line-continuation `\`) precede +// the origin, and the first origin token must start a new line. Aligning such +// partitions would glue the leading tokens onto the origin line via kInline +// cells (GitHub issue 2539). Leave them out of alignment instead. +static bool PartitionHasLeadingTokensBeforeForcedWrap( + const TokenPartitionTree &partition) { + const auto &uwline = partition.Value(); + const verible::Symbol *origin = uwline.Origin(); + if (origin == nullptr) return false; + + const auto ftokens = uwline.TokensRange(); + if (ftokens.empty()) return false; + + const verible::SyntaxTreeLeaf *first_leaf = verible::GetLeftmostLeaf(*origin); + if (first_leaf == nullptr) return false; + + const verible::TokenInfo &first_tree_token = first_leaf->get(); + auto ftoken_it = ftokens.begin(); + while (ftoken_it != ftokens.end() && + *(ftoken_it->token) != first_tree_token) { + ++ftoken_it; + } + if (ftoken_it == ftokens.begin() || ftoken_it == ftokens.end()) return false; + return ftoken_it->before.break_decision == verible::SpacingOptions::kMustWrap; +} + static bool IgnoreCommentsAndPreprocessingDirectives( const TokenPartitionTree &partition) { const auto &uwline = partition.Value(); @@ -159,6 +185,8 @@ // ignore lines containing only comments if (TokensAreAllCommentsOrAttributes(token_range)) return true; + if (PartitionHasLeadingTokensBeforeForcedWrap(partition)) return true; + // ignore partitions belonging to preprocessing directives return IsPreprocessorKeyword( verilog_tokentype(token_range.front().TokenEnum())); @@ -199,6 +227,8 @@ return true; } + if (PartitionHasLeadingTokensBeforeForcedWrap(partition)) return true; + // ignore nested structs/unions if (verible::FindFirstSubtree( partition.Value().Origin(), [](const Symbol &symbol) {
diff --git a/verible/verilog/formatting/formatter_class_package_test.cc b/verible/verilog/formatting/formatter_class_package_test.cc index 0c5e067..ee45c51 100644 --- a/verible/verilog/formatting/formatter_class_package_test.cc +++ b/verible/verilog/formatting/formatter_class_package_test.cc
@@ -96,6 +96,18 @@ "endinterface\n", }, { + // Keep space before explicit modport port name + "interface\tfoo ;" + "modport mp1(input .a(sig), output .b(sig));" + "endinterface", + "interface foo;\n" + " modport mp1(\n" + " input .a(sig),\n" + " output .b(sig)\n" + " );\n" + "endinterface\n", + }, + { // interface with long modport port names "interface\tfoo_if ;" "modport mp1\t( output a_long_output, input detailed_input_name);"
diff --git a/verible/verilog/formatting/formatter_issue_regression_test.cc b/verible/verilog/formatting/formatter_issue_regression_test.cc index c6f07aa..9b3bb7d 100644 --- a/verible/verilog/formatting/formatter_issue_regression_test.cc +++ b/verible/verilog/formatting/formatter_issue_regression_test.cc
@@ -135,6 +135,109 @@ } } +// Regression for https://github.com/chipsalliance/verible/issues/886: +// Packed dimensions with $clog2()/$bits() used to split the function header +// so ReshapeFittingSubpartitions dropped the port list. +TEST(FormatterEndToEndTest, FunctionHeaderPackedDimSystemCallKeepsPorts) { + static constexpr FormatterTestCase kTestCases[] = { + {// Original issue sample (default column_limit 100) + "package foo;\n" + " function some_large_return_type " + "[$clog2(some_large_contant_name)-1:0] " + "f_some_long_function( input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n", + "package foo;\n" + " function some_large_return_type " + "[$clog2(some_large_contant_name)-1:0] " + "f_some_long_function(\n" + " input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n"}, + {// Short names still keep ports and stay on one line + "package foo;\n" + " function logic [$clog2(N)-1:0] f(input int a, input int b);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n", + "package foo;\n" + " function logic [$clog2(N)-1:0] f(input int a, input int b);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n"}, + {// $bits() in packed dimensions + "package foo;\n" + " function some_large_return_type [$bits(some_large_contant_name)-1:0] " + "f_some_long_function(input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n", + "package foo;\n" + " function some_large_return_type [$bits(some_large_contant_name)-1:0] " + "f_some_long_function(\n" + " input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n"}, + {// Multi-argument system function in packed dimensions + "package foo;\n" + " function some_large_return_type " + "[$clog2(some_large_contant_name, WIDTH)-1:0] " + "f_some_long_function(input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n", + "package foo;\n" + " function some_large_return_type " + "[$clog2(some_large_contant_name, WIDTH)-1:0] " + "f_some_long_function(\n" + " input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n"}, + {// extern prototype + "class c;\n" + " extern function some_large_return_type " + "[$clog2(some_large_contant_name)-1:0] " + "f_some_long_function(input int parameter_1, input int parameter_2);\n" + "endclass\n", + "class c;\n" + " extern function some_large_return_type " + "[$clog2(some_large_contant_name)-1:0] " + "f_some_long_function(\n" + " input int parameter_1, input int parameter_2);\n" + "endclass\n"}, + }; + FormatStyle style; // default column_limit (100) + RunFormatterTestCases(style, kTestCases); +} + +TEST(FormatterEndToEndTest, FunctionHeaderPackedDimSystemCallWrapsArgs) { + // Tight column limit still keeps the ports (the original bug dropped them). + // The header itself is longer than 40 columns, so it wraps. + static constexpr FormatterTestCase kTestCases[] = { + {"package foo;\n" + " function some_large_return_type " + "[$clog2(some_large_contant_name)-1:0] " + "f_some_long_function( input int parameter_1, input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n", + "package foo;\n" + " function\n" + " some_large_return_type [$clog2(some_large_contant_name)-1\n" + " :0] f_some_long_function(\n" + " input int parameter_1,\n" + " input int parameter_2);\n" + " return 1;\n" + " endfunction\n" + "endpackage\n"}, + }; + RunFormatterTestCases40(kTestCases); +} + // 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). @@ -315,108 +418,27 @@ } } -// Regression for https://github.com/chipsalliance/verible/issues/886: -// Packed dimensions with $clog2()/$bits() used to split the function header -// so ReshapeFittingSubpartitions dropped the port list. -TEST(FormatterEndToEndTest, FunctionHeaderPackedDimSystemCallKeepsPorts) { +// Regression for https://github.com/chipsalliance/verible/issues/2539: +// A // comment followed by a line-continuation `\` before aligned ports must +// not abort in align.h, and must keep the comment on its own line. +TEST(FormatterEndToEndTest, PortListCommentWithLineContinuationDoesNotAbort) { static constexpr FormatterTestCase kTestCases[] = { - {// Original issue sample (default column_limit 100) - "package foo;\n" - " function some_large_return_type " - "[$clog2(some_large_contant_name)-1:0] " - "f_some_long_function( input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n", - "package foo;\n" - " function some_large_return_type " - "[$clog2(some_large_contant_name)-1:0] " - "f_some_long_function(\n" - " input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n"}, - {// Short names still keep ports and stay on one line - "package foo;\n" - " function logic [$clog2(N)-1:0] f(input int a, input int b);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n", - "package foo;\n" - " function logic [$clog2(N)-1:0] f(input int a, input int b);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n"}, - {// $bits() in packed dimensions - "package foo;\n" - " function some_large_return_type [$bits(some_large_contant_name)-1:0] " - "f_some_long_function(input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n", - "package foo;\n" - " function some_large_return_type [$bits(some_large_contant_name)-1:0] " - "f_some_long_function(\n" - " input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n"}, - {// Multi-argument system function in packed dimensions - "package foo;\n" - " function some_large_return_type " - "[$clog2(some_large_contant_name, WIDTH)-1:0] " - "f_some_long_function(input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n", - "package foo;\n" - " function some_large_return_type " - "[$clog2(some_large_contant_name, WIDTH)-1:0] " - "f_some_long_function(\n" - " input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n"}, - {// extern prototype - "class c;\n" - " extern function some_large_return_type " - "[$clog2(some_large_contant_name)-1:0] " - "f_some_long_function(input int parameter_1, input int parameter_2);\n" - "endclass\n", - "class c;\n" - " extern function some_large_return_type " - "[$clog2(some_large_contant_name)-1:0] " - "f_some_long_function(\n" - " input int parameter_1, input int parameter_2);\n" - "endclass\n"}, + {"module m (\n" + "//\\\n" + "input a\n" + ",input b\n" + ");\n" + "endmodule\n", + "module m (\n" + " //\\\n" + " input a\n" + " , input b\n" + ");\n" + "endmodule\n"}, }; FormatStyle style; // default column_limit (100) RunFormatterTestCases(style, kTestCases); } - -TEST(FormatterEndToEndTest, FunctionHeaderPackedDimSystemCallWrapsArgs) { - // Tight column limit still keeps the ports (the original bug dropped them). - // The header itself is longer than 40 columns, so it wraps. - static constexpr FormatterTestCase kTestCases[] = { - {"package foo;\n" - " function some_large_return_type " - "[$clog2(some_large_contant_name)-1:0] " - "f_some_long_function( input int parameter_1, input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n", - "package foo;\n" - " function\n" - " some_large_return_type [$clog2(some_large_contant_name)-1\n" - " :0] f_some_long_function(\n" - " input int parameter_1,\n" - " input int parameter_2);\n" - " return 1;\n" - " endfunction\n" - "endpackage\n"}, - }; - RunFormatterTestCases40(kTestCases); -} } // namespace } // namespace formatter } // namespace verilog
diff --git a/verible/verilog/formatting/formatter_module_test.cc b/verible/verilog/formatting/formatter_module_test.cc index f6e142b..c61da86 100644 --- a/verible/verilog/formatting/formatter_module_test.cc +++ b/verible/verilog/formatting/formatter_module_test.cc
@@ -3050,6 +3050,40 @@ RunFormatterTestCases40(kModuleFormatterTestCases); } +TEST(FormatterEndToEndTest, TernaryInsideSubscriptExpression_issue2597) { + static constexpr FormatterTestCase kTestCases[] = { + // Outside subscript + {"module foo ();\n" + "assign a = b > 1'h0 ? 1'h0 : c;\n" + "endmodule\n", + + "module foo ();\n" + " assign a = b > 1'h0 ? 1'h0 : c;\n" + "endmodule\n"}, + + // Inside subscript. + {"module foo ();\n" + "assign a = some_array[b > 1'h0 ? 1'h0 : c];\n" + "endmodule\n", + + "module foo ();\n" + " assign a = some_array[b > 1'h0 ? 1'h0 : c];\n" + "endmodule\n"}, + }; + + FormatStyle style; + style.indentation_spaces = 2; + 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); + // Require these test cases to be valid. + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + } // namespace } // namespace formatter } // namespace verilog
diff --git a/verible/verilog/formatting/token-annotator.cc b/verible/verilog/formatting/token-annotator.cc index fc6d2f0..d66042f 100644 --- a/verible/verilog/formatting/token-annotator.cc +++ b/verible/verilog/formatting/token-annotator.cc
@@ -117,7 +117,7 @@ return context.IsInsideFirst( {NodeEnum::kDimensionScalar, NodeEnum::kDimensionRange, NodeEnum::kDimensionSlice, NodeEnum::kCycleDelayRange}, - {}); + {NodeEnum::kConditionExpression}); // exclude } static bool IsAnySemicolon(const PreFormatToken &ftoken) { @@ -282,6 +282,12 @@ // TODO(fangism): Never insert trailing spaces before a newline. + // Modport port name separator, e.g. "input .a(" + if (right.TokenEnum() == '.' && + right_context.IsInside(NodeEnum::kModportSimplePort)) { + return {1, "Space before modport explicit port name '.'"}; + } + // Hierarchy examples: "a.b", "a::b" if (left.format_token_enum == FormatTokenType::hierarchy || right.format_token_enum == FormatTokenType::hierarchy) {
diff --git a/verible/verilog/formatting/token-annotator_test.cc b/verible/verilog/formatting/token-annotator_test.cc index 2adf936..c0f7e86 100644 --- a/verible/verilog/formatting/token-annotator_test.cc +++ b/verible/verilog/formatting/token-annotator_test.cc
@@ -2753,6 +2753,24 @@ {1, SpacingOptions::kUndecided}, }, + // Modport explicit port name, e.g. "input .a(sig)" + { + DefaultStyle, + {TK_input, "input"}, + {'.', "."}, + {/* any context */}, + {NodeEnum::kModportSimplePort}, + {1, SpacingOptions::kUndecided}, + }, + { + DefaultStyle, + {TK_output, "output"}, + {'.', "."}, + {/* any context */}, + {NodeEnum::kModportSimplePort}, + {1, SpacingOptions::kUndecided}, + }, + // Handle '->' as a unary prefix expression. { DefaultStyle, @@ -5501,6 +5519,31 @@ {NodeEnum::kUnpackedDimensions}, {1, SpacingOptions::kPreserve}, }, + { + // [b > 1'h0 ? 1'h0 : c] : space around '>' in ternary inside + // subscript + DefaultStyle, + verilog_tokentype::SymbolIdentifier, + "b", + " ", // 1 space originally + '>', + ">", + {NodeEnum::kDimensionScalar, NodeEnum::kConditionExpression}, + {NodeEnum::kDimensionScalar, NodeEnum::kConditionExpression}, + {1, SpacingOptions::kUndecided}, + }, + { + // [b > 1 ? 1 : c] : space before '?' in ternary inside subscript + DefaultStyle, + verilog_tokentype::TK_DecNumber, + "1", + " ", // 1 space originally + '?', + "?", + {NodeEnum::kDimensionScalar, NodeEnum::kConditionExpression}, + {NodeEnum::kDimensionScalar, NodeEnum::kConditionExpression}, + {1, SpacingOptions::kUndecided}, + }, }; int test_index = 0; for (const auto &test_case : kTestCases) {