Skip to content

Commit

Permalink
Fix crash in reshaping assignments with complex right-hand-sides.
Browse files Browse the repository at this point in the history 
Fix handling of comment indentation around constraint blocks in {}.
Allow constraint-item-list contexts to indent EOL-comments properly.

Follows similar pattern to function/macro calls' handling of ')'.

fixes #256

PiperOrigin-RevId: 308860224
  • Loading branch information
@fangism @hzeller
fangism authored and hzeller committed Apr 28, 2020
1 parent 727375b commit 0108308
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 10 deletions.
79 changes: 79 additions & 0 deletions verilog/formatting/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1887,6 +1887,18 @@ static const std::initializer_list<FormatterTestCase> kFormatterTestCases = {
" }\n"
"endclass\n",
},
{
"class foo; constraint c1_c{ //comment1\n"
"//comment2\n"
"//comment3\n"
"} endclass",
"class foo;\n"
" constraint c1_c { //comment1\n"
" //comment2\n"
" //comment3\n"
" }\n"
"endclass\n",
},

{"class foo;constraint c { "
"timer_enable dist { [ 8'h0 : 8'hfe ] :/ 90 , 8'hff :/ 10 }; "
Expand All @@ -1908,6 +1920,22 @@ static const std::initializer_list<FormatterTestCase> kFormatterTestCases = {
" }\n"
"endclass\n",
},
{
"class Foo; constraint if_c { if (z) {\n"
"//comment-a\n"
"soft x == y;\n"
"//comment-b\n"
"} } endclass\n",
"class Foo;\n"
" constraint if_c {\n"
" if (z) {\n"
" //comment-a\n" // properly indented
" soft x == y;\n"
" //comment-b\n" // properly indented
" }\n"
" }\n"
"endclass\n",
},

// class with empty parameter list
{"class foo #(); endclass",
Expand Down Expand Up @@ -2703,6 +2731,57 @@ static const std::initializer_list<FormatterTestCase> kFormatterTestCases = {
" end\n"
"endfunction\n",
},
{
// randomize-with call, with comments
"function f;"
"s = std::randomize() with {\n"
"// comment1\n"
"a == e;\n"
"// comment2\n"
"};"
"endfunction\n",
"function f;\n"
" s = std::randomize() with {\n"
" // comment1\n"
" a == e;\n"
" // comment2\n"
" };\n"
"endfunction\n",
},
{
// randomize-with call, with comments, one joined
"function f;"
"s = std::randomize() with {\n"
"// comment1\n"
"a == e;// comment2\n"
"};"
"endfunction\n",
"function f;\n"
" s = std::randomize() with {\n"
" // comment1\n"
" a == e; // comment2\n"
" };\n"
"endfunction\n",
},
{
// randomize-with call, with comment, and conditional
"function f;"
"s = std::randomize() with {\n"
"// comment\n"
"a == e;"
"if (x) {"
"a;"
"}"
"};"
"endfunction\n",
"function f;\n"
" s = std::randomize() with {\n"
" // comment\n"
" a == e;\n"
" if (x) {a;}\n" // TODO(fangism): consider expanding
" };\n"
"endfunction\n",
},

// module declaration test cases
{" module foo ; endmodule\n",
Expand Down
36 changes: 29 additions & 7 deletions verilog/formatting/tree_unwrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,8 @@ void TreeUnwrapper::InterChildNodeHook(const SyntaxTreeNode& node) {
case NodeEnum::kStatementList:
case NodeEnum::kPackageItemList:
case NodeEnum::kBlockItemStatementList:
case NodeEnum::kConstraintExpressionList:
case NodeEnum::kConstraintBlockItemList:
LookAheadBeyondCurrentNode();
break;
default: {
Expand Down Expand Up @@ -987,6 +989,13 @@ static bool PartitionIsCloseParen(const TokenPartitionTree& partition) {
return ((token_enum == ')') || (token_enum == MacroCallCloseToEndLine));
}

static bool PartitionIsCloseBrace(const TokenPartitionTree& partition) {
const auto ftokens = partition.Value().TokensRange();
if (ftokens.size() != 1) return false;
const auto token_enum = ftokens.front().TokenEnum();
return token_enum == '}';
}

static void AttachTrailingSemicolonToPreviousPartition(
TokenPartitionTree* partition) {
// Attach the trailing ';' partition to the previous sibling leaf.
Expand Down Expand Up @@ -1343,22 +1352,33 @@ void TreeUnwrapper::ReshapeTokenPartitions(
}
break;
}
case NodeEnum::kConstraintDeclaration: {
// TODO(fangism): kConstraintSet should be handled similarly with {}
if (partition.Children().size() == 2) {
auto& last = *ABSL_DIE_IF_NULL(partition.RightmostDescendant());
if (PartitionIsCloseBrace(last)) {
verible::MergeLeafIntoPreviousLeaf(&last);
}
}
break;
}

// This group of cases is temporary: simplify these during the
// rewrite/refactor of this function.
// See search-anchor: STATEMENT_TYPES
case NodeEnum::kNetVariableAssignment: // e.g. x=y
case NodeEnum::kBlockingAssignmentStatement: // id=expr
case NodeEnum::kNonblockingAssignmentStatement: // dest <= src;
case NodeEnum::kAssignModifyStatement: { // id+=expr
std::vector<size_t> offsets;
partition.FlattenOnlyChildrenWithChildren(&offsets);
case NodeEnum::kAssignModifyStatement: // id+=expr
{
VLOG(4) << "before moving semicolon:\n" << partition;
AttachTrailingSemicolonToPreviousPartition(&partition);
// Check body, for kSeqBlock, merge 'begin' with previous sibling
if (partition.Children().size() > 1) {
verible::MergeConsecutiveSiblings(&partition, 0);
VLOG(4) << "after merge siblings:\n" << partition;
// RHS may have been further partitioned, e.g. a macro call.
auto& children = partition.Children();
if (children.size() == 2 &&
children.front().Children().empty() /* left side */) {
verible::MergeLeafIntoNextLeaf(&children.front());
VLOG(4) << "after merge leaf (left-into-right):\n" << partition;
}
break;
}
Expand All @@ -1377,6 +1397,8 @@ void TreeUnwrapper::ReshapeTokenPartitions(
case NodeEnum::kProceduralContinuousAssignmentStatement:
case NodeEnum::kProceduralContinuousForceStatement:
case NodeEnum::kContinuousAssignmentStatement: { // e.g. assign a=0, b=2;
// TODO(fangism): group into above similar assignment statement cases?
// Cannot easily move now due to multiple-assignments.
partition.FlattenOnlyChildrenWithChildren();
VLOG(4) << "after flatten:\n" << partition;
AttachTrailingSemicolonToPreviousPartition(&partition);
Expand Down
121 changes: 119 additions & 2 deletions verilog/formatting/tree_unwrapper_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,23 @@ const TreeUnwrapperTestData kUnwrapModuleTestCases[] = {
L(0, {"endmodule"})),
},

{
"module multiple assignments with macro-call rvalue",
"module foob;\n"
"initial begin\n"
"assign z1 = `RVALUE(l1, r1);\n" // as statement item
"end\n"
"endmodule",
ModuleDeclaration(
0, L(0, {"module", "foob", ";"}),
ModuleItemList(
1, L(1, {"initial", "begin"}),
N(2, L(2, {"assign", "z1", "=", "`RVALUE", "("}),
MacroArgList(4, L(4, {"l1", ","}), L(4, {"r1", ")", ";"}))),
L(1, {"end"})),
L(0, {"endmodule"})),
},

{
"module with labeled statements",
"module labeled_statements;\n"
Expand Down Expand Up @@ -1936,6 +1953,24 @@ const TreeUnwrapperTestData kUnwrapModuleTestCases[] = {
L(0, {"endmodule"})),
},

{
"module procedural continuous force statements, macro rvalues",
"module proc_cont_forcer;\n"
"always begin\n"
"force x1 = `y1();\n"
"force x2 = `y2(f, g);\n"
"end\n"
"endmodule\n",
ModuleDeclaration(
0, L(0, {"module", "proc_cont_forcer", ";"}),
N(1, L(1, {"always", "begin"}),
N(2, L(2, {"force", "x1", "=", "`y1", "(", ")", ";"}),
N(2, L(2, {"force", "x2", "=", "`y2", "("}),
N(4, L(4, {"f", ","}), L(4, {"g", ")", ";"})))),
L(1, {"end"})),
L(0, {"endmodule"})),
},

{
"module with pair of procedural continuous de-assignment statements",
"module proc_cont_deassigner;\n"
Expand Down Expand Up @@ -2908,6 +2943,20 @@ const TreeUnwrapperTestData kClassTestCases[] = {
L(0, {"endclass"})),
},

{
"class with empty constraint, only comments",
"class Foo; constraint empty_c { //c1\n"
"//c2\n"
"} endclass",
ClassDeclaration(
0, L(0, {"class", "Foo", ";"}),
ConstraintDeclaration(
1, L(1, {"constraint", "empty_c", "{", "//c1"}), //
L(2, {"//c2"}), //
L(1, {"}"})),
L(0, {"endclass"})),
},

{
"class with multiple constraint declarations",
"class Foo; constraint empty1_c { } constraint empty2_c {} endclass",
Expand Down Expand Up @@ -2945,12 +2994,32 @@ const TreeUnwrapperTestData kClassTestCases[] = {
{
"class with conditional constraint set, constraint expression list",
"class Foo; constraint if_c { if (z) { soft x == y; } } endclass",
ClassDeclaration(
0, L(0, {"class", "Foo", ";"}),
ConstraintDeclaration(1, L(1, {"constraint", "if_c", "{"}),
N(2, L(2, {"if", "(", "z", ")", "{"}), //
L(3, {"soft", "x", "==", "y", ";"}), //
L(2, {"}"})),
L(1, {"}"})),
L(0, {"endclass"})),
},

{
"class with conditional constraint set, constraint exprs and comments",
"class Foo; constraint if_c { if (z) { //comment-w\n"
"//comment-x\n"
"soft x == y; //comment-y\n"
"//comment-z\n"
"} } endclass",
ClassDeclaration(
0, L(0, {"class", "Foo", ";"}),
ConstraintDeclaration(
1, L(1, {"constraint", "if_c", "{"}),
N(2, L(2, {"if", "(", "z", ")", "{"}),
L(3, {"soft", "x", "==", "y", ";"}), L(2, {"}"})),
N(2, L(2, {"if", "(", "z", ")", "{", "//comment-w"}), //
N(3, L(3, {"//comment-x"}), //
L(3, {"soft", "x", "==", "y", ";", "//comment-y"}), //
L(3, {"//comment-z"})),
L(2, {"}"})),
L(1, {"}"})),
L(0, {"endclass"})),
},
Expand Down Expand Up @@ -6338,6 +6407,54 @@ const TreeUnwrapperTestData kUnwrapFunctionTestCases[] = {
L(1, {"endfunction"})),
L(0, {"endclass"})),
},

{
"function with randomize-with call with comments",
"function f;\n"
"s = std::randomize() with {\n"
"// comment1\n"
"a == e;\n"
"// comment2\n"
"}; \n"
"endfunction\n",
FunctionDeclaration(
0, //
L(0, {"function", "f", ";"}),
N(1, //
L(1, {"s", "=", "std::randomize", "(", ")", "with", "{"}),
N(2, //
L(2, {"// comment1"}), //
L(2, {"a", "==", "e", ";"}), //
L(2, {"// comment2"})), //
L(1, {"}", ";"})),
L(0, {"endfunction"})),
},
{
"function with randomize-with call with leading comment",
"function f;\n"
"s = std::randomize() with {\n"
"// comment\n"
"a == e;\n"
"if (x) {\n"
"a;\n"
"}\n"
"}; \n"
"endfunction\n",
FunctionDeclaration(
0, //
L(0, {"function", "f", ";"}),
N(1, //
L(1, {"s", "=", "std::randomize", "(", ")", "with", "{"}),
N(2, //
L(2, {"// comment"}), //
L(2, {"a", "==", "e", ";"}),
N(2, //
L(2, {"if", "(", "x", ")", "{"}), //
L(3, {"a", ";"}), //
L(2, {"}"}))),
L(1, {"}", ";"})),
L(0, {"endfunction"})),
},
};

// Test that TreeUnwrapper produces correct UnwrappedLines from function tests
Expand Down
3 changes: 2 additions & 1 deletion verilog/parser/verilog.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1614,7 +1614,8 @@ constraint_block_item_list_opt
: constraint_block_item_list
{ $$ = move($1); }
| /* empty */
{ $$ = nullptr; }
/* create empty list */
{ $$ = MakeTaggedNode(N::kConstraintBlockItemList); }
;
preprocessor_balanced_constraint_block_item
: preprocessor_if_header constraint_block_item_list_opt
Expand Down

0 comments on commit 0108308

Please sign in to comment.