From c836a56396e913f8784cddb9bcb4a8772fde557a Mon Sep 17 00:00:00 2001 From: David Fang Date: Tue, 8 Sep 2020 13:46:07 -0700 Subject: [PATCH] Align module instance named parameters. Eliminate the old format_module_instantiations flag, now that we have fine-grain control over named parameter and named port connection sections of module instances. Slightly simplify the implementation of aligning named port connections. Handle comments inside named parameter actuals lists. (Fixes token partitioning w.r.t. comments, which also fixes formatter-caused output corruption, due to line munging.) PiperOrigin-RevId: 330578315 --- verilog/formatting/align.cc | 83 +++++++++++++++--- verilog/formatting/format_style.h | 5 ++ verilog/formatting/formatter.cc | 27 +++--- verilog/formatting/formatter_test.cc | 102 ++++++++++++++++++++-- verilog/formatting/tree_unwrapper.cc | 1 + verilog/formatting/tree_unwrapper_test.cc | 18 ++++ verilog/tools/formatter/verilog_format.cc | 11 +-- 7 files changed, 210 insertions(+), 37 deletions(-) diff --git a/verilog/formatting/align.cc b/verilog/formatting/align.cc index 1af1e1fe1..15357258a 100644 --- a/verilog/formatting/align.cc +++ b/verilog/formatting/align.cc @@ -100,6 +100,23 @@ static bool IgnoreWithinPortDeclarationPartitionGroup( return false; } +static bool IgnoreWithinActualNamedParameterPartitionGroup( + const TokenPartitionTree& partition) { + const auto& uwline = partition.Value(); + const auto token_range = uwline.TokensRange(); + CHECK(!token_range.empty()); + // ignore lines containing only comments + if (TokensAreAllComments(token_range)) return true; + + // ignore partitions belonging to preprocessing directives + if (IsPreprocessorKeyword(verilog_tokentype(token_range.front().TokenEnum()))) + return true; + + // ignore everything that isn't passing a parameter by name + return !verible::SymbolCastToNode(*uwline.Origin()) + .MatchesTag(NodeEnum::kParamByName); +} + static bool IgnoreWithinActualNamedPortPartitionGroup( const TokenPartitionTree& partition) { const auto& uwline = partition.Value(); @@ -149,16 +166,25 @@ static bool IgnoreWithinDataDeclarationPartitionGroup( return false; } -class ActualNamedPortColumnSchemaScanner : public ColumnSchemaScanner { +// This class marks up token-subranges in named parameter assignments for +// alignment. e.g. ".parameter_name(value_expression)" +class ActualNamedParameterColumnSchemaScanner : public ColumnSchemaScanner { public: - ActualNamedPortColumnSchemaScanner() = default; + ActualNamedParameterColumnSchemaScanner() = default; + void Visit(const SyntaxTreeNode& node) override { auto tag = NodeEnum(node.Tag().tag); VLOG(2) << __FUNCTION__ << ", node: " << tag << " at " << TreePathFormatter(Path()); switch (tag) { + case NodeEnum::kParamByName: { + // Always start first column right away + ReserveNewColumn(node, FlushLeft); + break; + } case NodeEnum::kParenGroup: - if (Context().DirectParentIs(NodeEnum::kActualNamedPort)) { + // Second column starts at the open parenthesis. + if (Context().DirectParentIs(NodeEnum::kParamByName)) { ReserveNewColumn(node, FlushLeft); } break; @@ -168,25 +194,40 @@ class ActualNamedPortColumnSchemaScanner : public ColumnSchemaScanner { TreeContextPathVisitor::Visit(node); VLOG(2) << __FUNCTION__ << ", leaving node: " << tag; } +}; - void Visit(const SyntaxTreeLeaf& leaf) override { - VLOG(2) << __FUNCTION__ << ", leaf: " << leaf.get() << " at " +// This class marks up token-subranges in named port connections for alignment. +// e.g. ".port_name(net_name)" +class ActualNamedPortColumnSchemaScanner : public ColumnSchemaScanner { + public: + ActualNamedPortColumnSchemaScanner() = default; + + void Visit(const SyntaxTreeNode& node) override { + auto tag = NodeEnum(node.Tag().tag); + VLOG(2) << __FUNCTION__ << ", node: " << tag << " at " << TreePathFormatter(Path()); - const int tag = leaf.get().token_enum(); switch (tag) { - case verilog_tokentype::SymbolIdentifier: + case NodeEnum::kActualNamedPort: { + // Always start first column right away + ReserveNewColumn(node, FlushLeft); + break; + } + case NodeEnum::kParenGroup: + // Second column starts at the open parenthesis. if (Context().DirectParentIs(NodeEnum::kActualNamedPort)) { - ReserveNewColumn(leaf, FlushLeft); + ReserveNewColumn(node, FlushLeft); } break; default: break; } - - VLOG(2) << __FUNCTION__ << ", leaving leaf: " << leaf.get(); + TreeContextPathVisitor::Visit(node); + VLOG(2) << __FUNCTION__ << ", leaving node: " << tag; } }; +// This class marks up token-subranges in port declarations for alignment. +// e.g. "input wire clk," class PortDeclarationColumnSchemaScanner : public ColumnSchemaScanner { public: PortDeclarationColumnSchemaScanner() = default; @@ -354,6 +395,8 @@ static std::vector GetConsecutiveDataDeclarationGroups( }); } +// This class marks up token-subranges in data declarations for alignment. +// e.g. "foo_pkg::bar_t [3:0] some_values;" // Much of the implementation of this scanner was based on // PortDeclarationColumnSchemaScanner. // Differences: @@ -485,7 +528,9 @@ class DataDeclarationColumnSchemaScanner : public ColumnSchemaScanner { bool new_column_after_open_bracket_ = false; }; -// For now, re-use the same column scanner as data/variable/net declarations. +// This class marks up token-subranges in class member variable (data +// declarations) for alignment. e.g. "const int [3:0] member_name;" For now, +// re-use the same column scanner as data/variable/net declarations. class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { public: ClassPropertyColumnSchemaScanner() = default; @@ -523,6 +568,9 @@ class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { } }; +// This class marks up token-subranges in formal parameter declarations for +// alignment. +// e.g. "localparam int Width = 5;" class ParameterDeclarationColumnSchemaScanner : public ColumnSchemaScanner { public: ParameterDeclarationColumnSchemaScanner() = default; @@ -659,6 +707,14 @@ static const verible::AlignedFormattingHandler kPortDeclarationAligner{ AlignmentCellScannerGenerator(), }; +static const verible::AlignedFormattingHandler kActualNamedParameterAligner{ + .extract_alignment_groups = &verible::GetSubpartitionsBetweenBlankLines, + .ignore_partition_predicate = + &IgnoreWithinActualNamedParameterPartitionGroup, + .alignment_cell_scanner = AlignmentCellScannerGenerator< + ActualNamedParameterColumnSchemaScanner>(), +}; + static const verible::AlignedFormattingHandler kActualNamedPortAligner{ .extract_alignment_groups = &verible::GetSubpartitionsBetweenBlankLines, .ignore_partition_predicate = &IgnoreWithinActualNamedPortPartitionGroup, @@ -719,6 +775,11 @@ void TabularAlignTokenPartitions(TokenPartitionTree* partition_ptr, [](const FormatStyle& vstyle) { return vstyle.port_declarations_alignment; }}}, + {NodeEnum::kActualParameterByNameList, + {kActualNamedParameterAligner, + [](const FormatStyle& vstyle) { + return vstyle.named_parameter_alignment; + }}}, {NodeEnum::kPortActualList, {kActualNamedPortAligner, [](const FormatStyle& vstyle) { diff --git a/verilog/formatting/format_style.h b/verilog/formatting/format_style.h index 4716eed3c..fcc84546c 100644 --- a/verilog/formatting/format_style.h +++ b/verilog/formatting/format_style.h @@ -42,6 +42,10 @@ struct FormatStyle : public verible::BasicFormatStyle { // and promote the control for this into AlignmentPolicy. bool format_module_instantiations = true; + // Control how named parameters (e.g. in module instances) are formatted. + // For internal testing purposes, this is default to kAlign. + AlignmentPolicy named_parameter_alignment = AlignmentPolicy::kAlign; + // Control how named ports (e.g. in module instances) are formatted. // Internal tests assume these are forced to kAlign. AlignmentPolicy named_port_alignment = AlignmentPolicy::kAlign; @@ -76,6 +80,7 @@ struct FormatStyle : public verible::BasicFormatStyle { void ApplyToAllAlignmentPolicies(AlignmentPolicy policy) { port_declarations_alignment = policy; + named_parameter_alignment = policy; named_port_alignment = policy; module_net_variable_alignment = policy; formal_parameters_alignment = policy; diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index b5680e910..97a668258 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -448,26 +448,25 @@ static verible::Interval DisableByteOffsetRange( } // Given control flags and syntax tree, selectively disable some ranges -// of text from formatting. +// of text from formatting. This provides an easy way to preserve spacing on +// selected syntax subtrees to reduce formatter harm while allowing +// development to progress. static void DisableSyntaxBasedRanges(ByteOffsetSet* disabled_ranges, const verible::Symbol& root, const FormatStyle& style, absl::string_view full_text) { - // Module-related sections: - for (const auto& match : FindAllModuleDeclarations(root)) { - if (!style.format_module_instantiations) { - const auto& instantiations = FindAllDataDeclarations(*match.match); - for (const auto& inst : instantiations) { - const auto& module_instances = FindAllGateInstances(*inst.match); - // Only suppress formatting if instances contains a module or - // gate-like instance with ports in parentheses. - if (module_instances.empty()) continue; - const auto inst_text = verible::StringSpanOfSymbol(*inst.match); - VLOG(4) << "disabled: " << inst_text; - disabled_ranges->Add(DisableByteOffsetRange(inst_text, full_text)); - } + /** + // Basic template: + if (!style.controlling_flag) { + for (const auto& match : FindAllSyntaxTreeNodeTypes(root)) { + // Refine search into specific subtrees, if applicable. + // Convert the spanning string_views into byte offset ranges to disable. + const auto inst_text = verible::StringSpanOfSymbol(*match.match); + VLOG(4) << "disabled: " << inst_text; + disabled_ranges->Add(DisableByteOffsetRange(inst_text, full_text)); } } + **/ } Status Formatter::Format(const ExecutionControl& control) { diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index ec8f3c93d..580beeb5b 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -6942,6 +6942,74 @@ TEST(FormatterEndToEndTest, AutoInferAlignment) { ");\n" "endmodule : pd\n"}, + // named parameter arguments + {"module mm ;\n" + "foo #(\n" + ".a(a),\n" + ".bb(bb)\n" + ")bar( );\n" + "endmodule:mm\n", + "module mm;\n" + " foo #(\n" + " .a (a),\n" // align doesn't add too many spaces, so align + " .bb(bb)\n" + " ) bar ();\n" + "endmodule : mm\n"}, + {"module mm ;\n" + "foo #(\n" + ".a(a),\n" + ".bbcccc(bb)\n" + ")bar( );\n" + "endmodule:mm\n", + "module mm;\n" + " foo #(\n" + " .a(a),\n" // align would add too many spaces, so flush-left + " .bbcccc(bb)\n" + " ) bar ();\n" + "endmodule : mm\n"}, + {"module mm ;\n" + "foo #(\n" + ".a(a ),\n" // user manually triggers alignment with excess spaces + ".bbcccc(bb)\n" + ")bar( );\n" + "endmodule:mm\n", + "module mm;\n" + " foo #(\n" + " .a (a),\n" // induced alignment + " .bbcccc(bb)\n" + " ) bar ();\n" + "endmodule : mm\n"}, + {"module mm ;\n" + "foo #(\n" + "//c1\n" // with comments (indented but not aligned) + ".a(a ),\n" // user manually triggers alignment with excess spaces + "//c2\n" + ".bbcccc(bb)\n" + "//c3\n" + ")bar( );\n" + "endmodule:mm\n", + "module mm;\n" + " foo #(\n" + " //c1\n" + " .a (a),\n" // induced alignment + " //c2\n" + " .bbcccc(bb)\n" + " //c3\n" + " ) bar ();\n" + "endmodule : mm\n"}, + {"module mm ;\n" + "foo #(\n" + ".a( (1 +2)),\n" // excess spaces, testing extra parentheses + ".bbcccc((c*d)+(e*f))\n" + ")bar( );\n" + "endmodule:mm\n", + "module mm;\n" + " foo #(\n" + " .a ((1 + 2)),\n" // induced alignment + " .bbcccc((c * d) + (e * f))\n" + " ) bar ();\n" + "endmodule : mm\n"}, + // named port connections {"module mm ;\n" "foo bar(\n" @@ -7256,7 +7324,7 @@ TEST(FormatterEndToEndTest, DisableModuleInstantiations) { "foo bar();" " endmodule\n", "module m;\n" - " foo bar();\n" // indentation still takes effect + " foo bar ();\n" // indentation still takes effect "endmodule\n"}, {"module m ;\t\n" "logic xyz;" @@ -7286,7 +7354,7 @@ TEST(FormatterEndToEndTest, DisableModuleInstantiations) { "foo bar( .baz(baz) );" " endmodule\n", "module m;\n" - " foo bar( .baz(baz) );\n" // indentation still takes effect + " foo bar (.baz(baz));\n" // indentation still takes effect "endmodule\n"}, {"module m ;\t\n" "foo bar(\n" @@ -7295,17 +7363,37 @@ TEST(FormatterEndToEndTest, DisableModuleInstantiations) { ");" " endmodule\n", "module m;\n" - " foo bar(\n" // indentation still takes effect - " .baz (baz ),\n" // named port connections preserved - " .blaaa(blaaa)\n" // named port connections preserved - ");\n" // this indentation remains untouched + " foo bar (\n" // indentation still takes effect + " .baz (baz ),\n" // named port connections preserved + " .blaaa(blaaa)\n" // named port connections preserved + " );\n" // this indentation is fixed + "endmodule\n"}, + {"module m ;\t\n" + "foo #( .baz(baz) ) bar();" // named parameters + " endmodule\n", + "module m;\n" + " foo #(.baz(baz)) bar ();\n" // indentation still takes effect + "endmodule\n"}, + {"module m ;\t\n" + "foo #(\n" + " .baz (baz ),\n" // example of user-manual alignment + " .blaaa(blaaa)\n" + ") bar( );" + " endmodule\n", + "module m;\n" + " foo #(\n" // indentation still takes effect + " .baz (baz ),\n" // named parameter arguments preserved + " .blaaa(blaaa)\n" // named parameter arguments preserved + " ) bar ();\n" // this indentation is fixed "endmodule\n"}, }; FormatStyle style; style.column_limit = 40; style.indentation_spaces = 2; style.wrap_spaces = 4; - style.format_module_instantiations = false; + // Testing preservation of spaces + style.named_parameter_alignment = AlignmentPolicy::kPreserve; + style.named_port_alignment = AlignmentPolicy::kPreserve; for (const auto& test_case : kTestCases) { VLOG(1) << "code-to-format:\n" << test_case.input << ""; std::ostringstream stream; diff --git a/verilog/formatting/tree_unwrapper.cc b/verilog/formatting/tree_unwrapper.cc index e8ffa580c..b93ef96fc 100644 --- a/verilog/formatting/tree_unwrapper.cc +++ b/verilog/formatting/tree_unwrapper.cc @@ -517,6 +517,7 @@ void TreeUnwrapper::InterChildNodeHook(const SyntaxTreeNode& node) { // TODO(fangism): cover all other major lists case NodeEnum::kFormalParameterList: case NodeEnum::kPortDeclarationList: + case NodeEnum::kActualParameterByNameList: case NodeEnum::kPortActualList: // case NodeEnum::kPortList: // TODO(fangism): for task/function ports case NodeEnum::kModuleItemList: diff --git a/verilog/formatting/tree_unwrapper_test.cc b/verilog/formatting/tree_unwrapper_test.cc index 3078c1e7e..d8c68f714 100644 --- a/verilog/formatting/tree_unwrapper_test.cc +++ b/verilog/formatting/tree_unwrapper_test.cc @@ -987,6 +987,24 @@ const TreeUnwrapperTestData kUnwrapModuleTestCases[] = { L(0, {"endmodule"})), }, + { + "module instance with named parameter interleaved among EOL comments", + "module tryme;" + "foo #(//c1\n//c2\n.N(5), //c3\n//c4\n.M(6)//c5\n//c6\n) a;" + "endmodule", + ModuleDeclaration( + 0, L(0, {"module", "tryme", ";"}), + Instantiation(1, L(1, {"foo", "#", "(", "//c1"}), + N(3, // + L(3, {"//c2"}), // + L(3, {".", "N", "(", "5", ")", ",", "//c3"}), // + L(3, {"//c4"}), // + L(3, {".", "M", "(", "6", ")", "//c5"}), // + L(3, {"//c6"})), // + L(1, {")", "a", ";"})), + L(0, {"endmodule"})), + }, + { "module with single instance and positional port actuals", "module got_ports;" diff --git a/verilog/tools/formatter/verilog_format.cc b/verilog/tools/formatter/verilog_format.cc index c8f7a8f11..f50488614 100644 --- a/verilog/tools/formatter/verilog_format.cc +++ b/verilog/tools/formatter/verilog_format.cc @@ -129,6 +129,9 @@ ABSL_FLAG(int, max_search_states, 100000, ABSL_FLAG(AlignmentPolicy, port_declarations_alignment, AlignmentPolicy::kInferUserIntent, "Format port declarations: {align,flush-left,preserve,infer}"); +ABSL_FLAG(AlignmentPolicy, named_parameter_alignment, + AlignmentPolicy::kInferUserIntent, + "Format named actual parameters: {align,flush-left,preserve,infer}"); ABSL_FLAG(AlignmentPolicy, named_port_alignment, AlignmentPolicy::kInferUserIntent, "Format named port connections: {align,flush-left,preserve,infer}"); @@ -143,9 +146,6 @@ ABSL_FLAG(AlignmentPolicy, class_member_variables_alignment, AlignmentPolicy::kInferUserIntent, "Format class member variables: {align,flush-left,preserve,infer}"); -ABSL_FLAG(bool, format_module_instantiations, true, - "If true, format module instantiations (data declarations), " - "else leave them unformatted. This is a short-term workaround."); ABSL_FLAG(bool, try_wrap_long_lines, false, "If true, let the formatter attempt to optimize line wrapping " "decisions where wrapping is needed, else leave them unformatted. " @@ -203,12 +203,13 @@ static bool formatOneFile(absl::string_view filename, absl::GetFlag(FLAGS_verify_convergence); // formatting style flags - format_style.format_module_instantiations = - absl::GetFlag(FLAGS_format_module_instantiations); format_style.try_wrap_long_lines = absl::GetFlag(FLAGS_try_wrap_long_lines); + // various alignment control format_style.port_declarations_alignment = absl::GetFlag(FLAGS_port_declarations_alignment); + format_style.named_parameter_alignment = + absl::GetFlag(FLAGS_named_parameter_alignment); format_style.named_port_alignment = absl::GetFlag(FLAGS_named_port_alignment); format_style.module_net_variable_alignment =