Skip to content

Commit

Permalink
Align module instance named parameters.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@fangism @hzeller
fangism authored and hzeller committed Sep 8, 2020
1 parent b8b6b6e commit c836a56
Show file tree
Hide file tree
Showing 7 changed files with 210 additions and 37 deletions.
83 changes: 72 additions & 11 deletions verilog/formatting/align.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -354,6 +395,8 @@ static std::vector<TokenPartitionRange> 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:
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -659,6 +707,14 @@ static const verible::AlignedFormattingHandler kPortDeclarationAligner{
AlignmentCellScannerGenerator<PortDeclarationColumnSchemaScanner>(),
};

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,
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions verilog/formatting/format_style.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
27 changes: 13 additions & 14 deletions verilog/formatting/formatter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,26 +448,25 @@ static verible::Interval<int> 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) {
Expand Down
102 changes: 95 additions & 7 deletions verilog/formatting/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;"
Expand Down Expand Up @@ -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"
Expand All @@ -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 << "<EOF>";
std::ostringstream stream;
Expand Down
1 change: 1 addition & 0 deletions verilog/formatting/tree_unwrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
18 changes: 18 additions & 0 deletions verilog/formatting/tree_unwrapper_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;"
Expand Down
Loading

0 comments on commit c836a56

Please sign in to comment.