Skip to content

Commit

Permalink
Merge pull request #1917 from antmicro/fix-divergence-in-macro-format…
Browse files Browse the repository at this point in the history 
…ting

Fix macro formatting divergence and splitting issues
  • Loading branch information
@glatosinski
glatosinski authored Jul 4, 2023
2 parents 8d25416 + 7530f9f commit 63352e3
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 26 deletions.
31 changes: 30 additions & 1 deletion verilog/formatting/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3717,6 +3717,15 @@ static constexpr FormatterTestCase kFormatterTestCases[] = {
" lbl : y = w;\n" // TODO(fangism): no space before ':'
" endtask\n"
"endclass\n"},
// task with macro call
{"module m1;\ntask automatic t1();\n"
"t2(`R1(1)+ 8);endtask : t1\nendmodule",
"module m1;\n"
" task automatic t1();\n"
" t2(`R1(1) + 8);\n"
" endtask : t1\n"
"endmodule\n"},

// tasks with control statements
{"class c; task automatic waiter;"
"if (count == 0) begin #0; return;end "
Expand Down Expand Up @@ -5564,7 +5573,27 @@ static constexpr FormatterTestCase kFormatterTestCases[] = {
" x = 2;\n"
" end\n"
"endmodule\n"},

// Macro calls inside [...]
{"module foo;\nlogic [`BAR(1)\n+2] foo;\nendmodule",
"module foo;\n"
" logic [`BAR(1)\n"
"+2] foo;\n"
"endmodule\n"},
{"module foo;\nlogic [`BAR(1)+2] foo;\nendmodule",
"module foo;\n"
" logic [`BAR(1)+2] foo;\n"
"endmodule\n"},
{"module foo ();\n function bar();\n "
"logic [12:34] data[`AAAAAAAAAAAAAAAAAAAAAAAA(1) : "
"`BBBBBBBBBBBBBBBBBBBBBBBB(2) + 3];\n "
"endfunction\nendmodule",
"module foo ();\n"
" function bar();\n"
" logic [12:34] data[\n"
" `AAAAAAAAAAAAAAAAAAAAAAAA(1) :\n"
" `BBBBBBBBBBBBBBBBBBBBBBBB(2) + 3];\n"
" endfunction\n"
"endmodule\n"},
{
// test that alternate top-syntax mode works
"// verilog_syntax: parse-as-module-body\n"
Expand Down
16 changes: 2 additions & 14 deletions verilog/formatting/token_annotator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,18 +161,6 @@ static WithReason<int> SpacesRequiredBetween(
"and \"{y}\" over \"{ y }\"."};
}

// For now, leave everything inside [dimensions] alone.
if (InDeclaredDimensions(right_context)) {
// ... except for the spacing before '[' and around ':',
// which are covered elsewhere.
if (right.TokenEnum() != '[' && left.TokenEnum() != ':' &&
right.TokenEnum() != ':') {
return {kUnhandledSpacesRequired,
"Leave [expressions] inside scalar and range dimensions alone "
"(for now)."};
}
}

// Unary operators (context-sensitive)
if (IsUnaryPrefixExpressionOperand(left, right_context) &&
(left.format_token_enum != FormatTokenType::binary_operator ||
Expand Down Expand Up @@ -441,7 +429,7 @@ static WithReason<int> SpacesRequiredBetween(
if (InRangeLikeContext(right_context)) {
int spaces = right.OriginalLeadingSpaces().length();
if (spaces > 1) {
// If ExcessSpaces returns 0 if there was a newline - prevents
// ExcessSpaces returns 0 if there was a newline - prevents
// counting indentation as spaces
spaces = right.ExcessSpaces() ? 1 : 0;
}
Expand Down Expand Up @@ -883,7 +871,7 @@ static WithReason<SpacingOptions> BreakDecisionBetween(

if (left.TokenEnum() == verilog_tokentype::MacroCallCloseToEndLine) {
if (!IsComment(FormatTokenType(right.format_token_enum)) &&
!IsAnySemicolon(right)) {
!IsAnySemicolon(right) && !InRangeLikeContext(left_context)) {
return {SpacingOptions::kMustWrap,
"Macro-closing ')' should end its own line except for comments "
"nad ';'."};
Expand Down
55 changes: 44 additions & 11 deletions verilog/formatting/tree_unwrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,14 +842,14 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
case NodeEnum::kCovergroupHeader:
case NodeEnum::kModportDeclaration:
case NodeEnum::kInstantiationType:
case NodeEnum::kRegisterVariable:
case NodeEnum::kVariableDeclarationAssignment:
case NodeEnum::kCaseItem:
case NodeEnum::kDefaultItem:
case NodeEnum::kCaseInsideItem:
case NodeEnum::kCasePatternItem:
case NodeEnum::kGenerateCaseItem:
case NodeEnum::kGateInstance:
case NodeEnum::kRegisterVariable:
case NodeEnum::kGenerateIfClause:
case NodeEnum::kGenerateElseClause:
case NodeEnum::kGenerateIfHeader:
Expand All @@ -864,7 +864,6 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
VisitIndentedSection(node, 0, PartitionPolicyEnum::kFitOnLineElseExpand);
break;
}

// The following cases will always expand into their constituent
// partitions:
case NodeEnum::kModuleDeclaration:
Expand Down Expand Up @@ -1109,8 +1108,8 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
NodeEnum::kIfBody,
NodeEnum::kIfClause,
})) &&
!(Context().IsInside(NodeEnum::kNetVariableAssignment) //
));
!(Context().IsInside(NodeEnum::kNetVariableAssignment)) &&
!(Context().IsInside(NodeEnum::kUnpackedDimensions)));
};

// TODO(mglb): Format non-standalone calls with layout optimizer.
Expand All @@ -1126,6 +1125,8 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
PushContextHint(ContextHint::kInsideStandaloneMacroCall);
VisitIndentedSection(node, indent, PartitionPolicyEnum::kStack);
} else {
// TODO(jbylicki): Check if there is a possibility of
// using kFitOnLineElseExpand - seems to have no effect
VisitIndentedSection(node, indent,
PartitionPolicyEnum::kAppendFittingSubPartitions);
}
Expand Down Expand Up @@ -2714,13 +2715,10 @@ void TreeUnwrapper::ReshapeTokenPartitions(
if (subnode.MatchesTagAnyOf({NodeEnum::kMethodCallExtension,
NodeEnum::kParenGroup,
NodeEnum::kRandomizeMethodCallExtension})) {
if (partition.Value().PartitionPolicy() ==
PartitionPolicyEnum::kAppendFittingSubPartitions) {
auto& last = RightmostDescendant(partition);
if (PartitionStartsWithCloseParen(last) ||
PartitionStartsWithSemicolon(last)) {
verible::MergeLeafIntoPreviousLeaf(&last);
}
auto& last = RightmostDescendant(partition);
if (PartitionStartsWithCloseParen(last) ||
PartitionStartsWithSemicolon(last)) {
verible::MergeLeafIntoPreviousLeaf(&last);
}
}
break;
Expand Down Expand Up @@ -3123,6 +3121,41 @@ void TreeUnwrapper::ReshapeTokenPartitions(
break;
}

case NodeEnum::kRegisterVariable:
case NodeEnum::kInstantiationType: {
size_t partition_size = partition.Children().size();
// Partition with complex expressions inside dimensions have at least 4
// children, so they are filtered
if (partition_size < 4) break;
// Then either packed or unpacked dimensions are extracted from their
// positions
auto subnode = verible::GetSubtreeAsNode(node, tag, 1);
if (!subnode ||
NodeEnum(subnode->Tag().tag) != NodeEnum::kUnpackedDimensions) {
subnode = verible::GetSubtreeAsNode(node, tag, 0);
subnode = verible::GetSubtreeAsNode(*subnode, subnode->Tag().tag, 3);
if (!subnode ||
NodeEnum(subnode->Tag().tag) != NodeEnum::kPackedDimensions) {
break;
}
}
// Next the colon is located and attached to the first dimension
auto& colon = partition.Children()[partition_size - 3];
// And the colon is checked to be an actual colon
if (colon.Value().TokensRange().begin()->Text() != ":") break;
AttachSeparatorToPreviousOrNextPartition(&colon);
// And if everything went smoothly, the latter dimensions are attached to
// their predecessors
verible::MergeLeafIntoPreviousLeaf(
&partition.Children()[partition_size - 2]);
// And flattened so that the breaks contain the whole dimension range in
// one line
verible::FlattenOneChild(partition, partition_size - 3);
verible::MergeLeafIntoPreviousLeaf(
&partition.Children()[partition_size - 2]);
break;
}

default:
break;
}
Expand Down

0 comments on commit 63352e3

Please sign in to comment.