Skip to content

Commit

Permalink
PR #367: Align module instance named port connections (for #313)
Browse files Browse the repository at this point in the history 
GitHub PR #367

Copybara import of the project:

  - b2f04ad Merge remote-tracking branch 'google/master' into issue_313 by Ciprian <ciprian.antoci@gmail.com>
  - ef5df2d Initial implementation on what could be a solution to #313 by Ciprian <ciprian.antoci@gmail.com>
  - f53d4bf Merge remote-tracking branch 'google/master' into issue_313 by Ciprian <ciprian.antoci@gmail.com>
  - 8e0e5d8 Correction to module instantiation named ports tabular al... by Ciprian <ciprian.antoci@gmail.com>
  - 80331cc Implementing requested changes. by Ciprian <ciprian.antoci@gmail.com>

Closes #367
issues #313

PiperOrigin-RevId: 322183219
  • Loading branch information
@Ciprian167 @hzeller
Ciprian167 authored and hzeller committed Jul 20, 2020
1 parent fd1cbad commit 100c0af
Show file tree
Hide file tree
Showing 4 changed files with 293 additions and 1 deletion.
88 changes: 88 additions & 0 deletions verilog/formatting/align.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ static bool TokensAreAllComments(const T& tokens) {
}) == tokens.end();
}

template <class T>
static bool TokensHaveParenthesis(const T& tokens) {
return std::any_of(tokens.begin(), tokens.end(),
[](const typename T::value_type& token) {
return token.TokenEnum() == '(';
});
}

static bool IgnorePortDeclarationPartition(
const TokenPartitionTree& partition) {
const auto& uwline = partition.Value();
Expand All @@ -86,6 +94,78 @@ static bool IgnorePortDeclarationPartition(
return false;
}

static bool IgnoreActualNamedPortPartition(
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 wildcard connections .*
if (verilog_tokentype(token_range.front().TokenEnum()) ==
verilog_tokentype::TK_DOTSTAR) {
return true;
}

// ignore implicit connections .aaa
if (verible::SymbolCastToNode(*uwline.Origin())
.MatchesTag(NodeEnum::kActualNamedPort) &&
!TokensHaveParenthesis(token_range)) {
return true;
}

// ignore positional port connections
if (verible::SymbolCastToNode(*uwline.Origin())
.MatchesTag(NodeEnum::kActualPositionalPort)) {
return true;
}

return false;
}

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());
switch (tag) {
case NodeEnum::kParenGroup:
if (Context().DirectParentIs(NodeEnum::kActualNamedPort)) {
ReserveNewColumn(node, FlushLeft);
}
break;
default:
break;
}
TreeContextPathVisitor::Visit(node);
VLOG(2) << __FUNCTION__ << ", leaving node: " << tag;
}

void Visit(const SyntaxTreeLeaf& leaf) override {
VLOG(2) << __FUNCTION__ << ", leaf: " << leaf.get() << " at "
<< TreePathFormatter(Path());
const int tag = leaf.get().token_enum();
switch (tag) {
case verilog_tokentype::SymbolIdentifier:
if (Context().DirectParentIs(NodeEnum::kActualNamedPort)) {
ReserveNewColumn(leaf, FlushLeft);
}
break;
default:
break;
}

VLOG(2) << __FUNCTION__ << ", leaving leaf: " << leaf.get();
}
};

class PortDeclarationColumnSchemaScanner : public ColumnSchemaScanner {
public:
PortDeclarationColumnSchemaScanner() = default;
Expand Down Expand Up @@ -230,6 +310,13 @@ static const verible::AlignedFormattingHandler kPortDeclarationAligner{
AlignmentCellScannerGenerator<PortDeclarationColumnSchemaScanner>(),
};

static const verible::AlignedFormattingHandler kActualNamedPortAligner{
.extract_alignment_groups = &verible::GetSubpartitionsBetweenBlankLines,
.ignore_partition_predicate = &IgnoreActualNamedPortPartition,
.alignment_cell_scanner =
AlignmentCellScannerGenerator<ActualNamedPortColumnSchemaScanner>(),
};

void TabularAlignTokenPartitions(TokenPartitionTree* partition_ptr,
std::vector<PreFormatToken>* ftokens,
absl::string_view full_text,
Expand All @@ -249,6 +336,7 @@ void TabularAlignTokenPartitions(TokenPartitionTree* partition_ptr,
static const auto* kAlignHandlers =
new std::map<NodeEnum, verible::AlignedFormattingHandler>{
{NodeEnum::kPortDeclarationList, kPortDeclarationAligner},
{NodeEnum::kPortActualList, kActualNamedPortAligner},
};
const auto handler_iter = kAlignHandlers->find(NodeEnum(node->Tag().tag));
if (handler_iter == kAlignHandlers->end()) return;
Expand Down
183 changes: 183 additions & 0 deletions verilog/formatting/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5268,6 +5268,189 @@ static const std::initializer_list<FormatterTestCase> kFormatterTestCases = {
"JgQLBG == 4'h0;, \"foo\")\n"
"endtask\n",
},

// module instantiation named ports tabular alignment
//{// module instantiation with no port, only comments
// "module m;\n"
// "foo bar(\n"
// "\t//comment1\n"
// "//comment2\n"
// ");\n"
// "endmodule\n",
// "module m;\n"
// " foo bar (\n"
// " //comment1\n"
// " //comment2\n"
// " );\n"
// "endmodule\n"
//},
{// all named ports
"module m;\n"
"foo bar(.a(a), .aa(aa), .aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aa (aa),\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{// named ports left unconnected
"module m;\n"
"foo bar(.a(), .aa(), .aaa());\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (),\n"
" .aa (),\n"
" .aaa()\n"
" );\n"
"endmodule\n"},
{// multiple named ports groups separated by blank line
"module m;\n"
"foo bar(.a(a), .aaa(aaa),\n\n .b(b), .bbbbbb(bbbbb));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aaa(aaa),\n"
"\n"
" .b (b),\n"
" .bbbbbb(bbbbb)\n"
" );\n"
"endmodule\n"},
{// named ports with concatenation
"module m;\n"
"foo bar(.a(a), .aaa({a,b,c}));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aaa({a, b, c})\n"
" );\n"
"endmodule\n"},
{// name ports with slices
"module m;\n"
"foo bar(.a(a), .aaa(q[r:s]));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aaa(q[r:s])\n"
" );\n"
"endmodule\n"},
{// named ports with pre-proc directives
"module m;\n"
"foo bar(.a(a), `ifdef MACRO .aa(aa), `endif .aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
"`ifdef MACRO\n"
" .aa (aa),\n"
"`endif\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{// named ports with macros
"module m;\n"
"foo bar(.a(a), .aa(aa[`RANGE]), .aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aa (aa[`RANGE]),\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{"module m;\n"
"foo bar(.a(a), .AA, .aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .AA,\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{// name ports with comments
"module m;\n"
"foo bar(.a(a), .aa(aa)/*comment*/, .aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aa (aa) /*comment*/,\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{"module m;\n"
"foo bar(.a(a),//comment1\n .aaa(aaa)//comment2\n);\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a), //comment1\n"
" .aaa(aaa) //comment2\n"
" );\n"
"endmodule\n"},
{"module m;\n"
"foo bar(.a(a),\n"
" //.aa(aa),\n"
".aaa(aaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" //.aa(aa),\n"
" .aaa(aaa)\n"
" );\n"
"endmodule\n"},
{// module instantiation with all implicit connections
"module m;\n"
"foo bar(.a, .aa, .aaaaa);\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a,\n"
" .aa,\n"
" .aaaaa\n"
" );\n"
"endmodule\n"},
{// named ports corssed with implicit connections
"module m;\n"
"foo bar(.a(a), .aa, .aaaaa(aaaaa));\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aa,\n"
" .aaaaa(aaaaa)\n"
" );\n"
"endmodule\n"},
{// named ports corssed with wildcard connections
"module m;\n"
"foo bar(.a(a), .aaa(aaa), .*);\n"
"endmodule\n",
"module m;\n"
" foo bar (\n"
" .a (a),\n"
" .aaa(aaa),\n"
" .*\n"
" );\n"
"endmodule\n"},
//{
// "module m;\n"
// "foo bar(.a(a), .aa(aa), .* , .aaa(aaa));\n"
// "endmodule\n",
// "module m;\n"
// " foo bar (\n"
// " .a (a),\n"
// " .aa (aa),\n"
// " .*,\n"
// " .aaa(aaa)\n"
// " );\n"
// "endmodule\n"
//},
};

// Tests that formatter produces expected results, end-to-end.
Expand Down
3 changes: 2 additions & 1 deletion verilog/formatting/tree_unwrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ void TreeUnwrapper::InterChildNodeHook(const SyntaxTreeNode& node) {
switch (tag) {
// TODO(fangism): cover all other major lists
case NodeEnum::kPortDeclarationList:
case NodeEnum::kPortActualList:
// case NodeEnum::kPortList: // TODO(fangism): for task/function ports
case NodeEnum::kModuleItemList:
case NodeEnum::kGenerateItemList:
Expand Down Expand Up @@ -929,7 +930,7 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
{
const int indent = suppress_indentation ? 0 : style_.wrap_spaces;
const auto policy = Context().IsInside(NodeEnum::kDataDeclaration)
? PartitionPolicyEnum::kAlwaysExpand
? PartitionPolicyEnum::kTabularAlignment
: PartitionPolicyEnum::kFitOnLineElseExpand;
VisitIndentedSection(node, indent, policy);
break;
Expand Down
20 changes: 20 additions & 0 deletions verilog/formatting/tree_unwrapper_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,26 @@ const TreeUnwrapperTestData kUnwrapModuleTestCases[] = {
L(0, {"endmodule"})),
},

{
"module with instances with commented named ports",
"module named_ports;"
"foo bar(\n"
".a(a),\n"
"//.aa(aa),\n"
".aaa(aaa)\n"
");"
"endmodule",
ModuleDeclaration(
0, L(0, {"module", "named_ports", ";"}),
Instantiation(1, L(1, {"foo", "bar", "("}),
PortActualList(3, //
L(3, {".", "a", "(", "a", ")", ","}),
L(3, {"//.aa(aa),"}),
L(3, {".", "aaa", "(", "aaa", ")"})),
L(1, {")", ";"})),
L(0, {"endmodule"})),
},

{
"module interface ports",
"module foo ("
Expand Down

0 comments on commit 100c0af

Please sign in to comment.