PR #356: macro definition partitioning

Fixes #252 GitHub PR #356 Copybara import of the project: - f62f61a Fixed partitioning of formal arguments in macro definition by Wojciech Tatarski <wtatarski@antmicro.com> Closes #356 PiperOrigin-RevId: 320625972
chipsalliance · Jul 10, 2020 · b458ff6 · b458ff6
1 parent 19dade3
commit b458ff6
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 5 deletions.
diff --git a/verilog/CST/verilog_nonterminals.h b/verilog/CST/verilog_nonterminals.h
@@ -271,6 +271,8 @@ enum class NodeEnum {
   kMacroArgList,
   kMacroCall,
   kMacroGenericItem,
+  kMacroFormalParameterList,
+  kMacroFormalArg,
   kAssignmentPattern,
   kPatternExpression,
   kMinTypMaxList,

diff --git a/verilog/formatting/tree_unwrapper.cc b/verilog/formatting/tree_unwrapper.cc
@@ -644,7 +644,6 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
     case NodeEnum::kPackageImportDeclaration:
     // TODO(fangism): case NodeEnum::kDPIExportItem:
     case NodeEnum::kPreprocessorInclude:
-    case NodeEnum::kPreprocessorDefine:
     case NodeEnum::kPreprocessorUndef:
     case NodeEnum::kModuleDeclaration:
     case NodeEnum::kProgramDeclaration:
@@ -680,6 +679,7 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
     case NodeEnum::kActualPositionalPort:
     case NodeEnum::kAssertionVariableDeclaration:
     case NodeEnum::kPortItem:
+    case NodeEnum::kMacroFormalArg:
     case NodeEnum::kPropertyDeclaration:
     case NodeEnum::kSequenceDeclaration:
     case NodeEnum::kPort:
@@ -844,6 +844,7 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
       // The following constructs wish to use the partition policy of appending
       // trailing subpartitions greedily as long as they fit, wrapping as
       // needed.
+    case NodeEnum::kPreprocessorDefine:
     case NodeEnum::kClassConstructorPrototype:
     case NodeEnum::kTaskHeader:
     case NodeEnum::kFunctionHeader:
@@ -893,6 +894,7 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
     }
 
       // Add a level of grouping that is treated as wrapping.
+    case NodeEnum::kMacroFormalParameterList:
     case NodeEnum::kMacroArgList:
     case NodeEnum::kForSpec:
     case NodeEnum::kModportSimplePortsDeclaration:
@@ -990,7 +992,7 @@ static bool PartitionIsCloseParenSemi(const TokenPartitionTree& partition) {
 
 static bool PartitionIsCloseParen(const TokenPartitionTree& partition) {
   const auto ftokens = partition.Value().TokensRange();
-  if (ftokens.size() != 1) return false;
+  if (ftokens.empty()) return false;
   const auto token_enum = ftokens.front().TokenEnum();
   return ((token_enum == ')') || (token_enum == MacroCallCloseToEndLine));
 }
@@ -1361,6 +1363,15 @@ void TreeUnwrapper::ReshapeTokenPartitions(
         break;
       }
 
+    case NodeEnum::kPreprocessorDefine: {
+      auto& last = *ABSL_DIE_IF_NULL(partition.RightmostDescendant());
+      // TODO(fangism): why does test fail without this clause?
+      if (PartitionIsCloseParen(last)) {
+        verible::MergeLeafIntoPreviousLeaf(&last);
+      }
+      break;
+    }
+
     case NodeEnum::kMacroCall: {
       // If there are no call args, join the '(' and ')' together.
       if (MacroCallArgsIsEmpty(GetMacroCallArgs(node))) {

diff --git a/verilog/formatting/tree_unwrapper_test.cc b/verilog/formatting/tree_unwrapper_test.cc
@@ -4118,6 +4118,98 @@ const TreeUnwrapperTestData kUnwrapPreprocessorTestCases[] = {
             L(0, {"endmodule"})),
     },
 
+    {
+        "Partitioning formal argument `define",
+        "`define FOO(BAR)\n",
+        N(0, L(0, {"`define", "FOO", "("}), L(2, {"BAR", ")", ""})),
+    },
+
+    {
+        "Partitioning formal argument `define with body definition",
+        "`define FOO(BAR) body_def\n",
+        N(0, L(0, {"`define", "FOO", "("}), L(2, {"BAR", ")", "body_def"})),
+    },
+
+    {
+        "Partitioning formal arguments in `define",
+        "`define FOO(BAR1, BAR2, BAR3)\n",
+        N(0, L(0, {"`define", "FOO", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", ""}))),
+    },
+
+    {
+        "Partitioning formal arguments in `define with body definition",
+        "`define FOO(BAR1, BAR2, BAR3) definition_body\n",
+        N(0, L(0, {"`define", "FOO", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", "definition_body"}))),
+    },
+
+    {
+        "Partitioning formal arguments in consecutive `define's",
+        "`define FOO1(BAR1, BAR2, BAR3)\n"
+        "`define FOO2(BAR1, BAR2, BAR3)\n",
+        N(0, L(0, {"`define", "FOO1", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", ""}))),
+        N(0, L(0, {"`define", "FOO2", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", ""}))),
+    },
+
+    {
+        "Partitioning formal arguments in consecutive `define's with body def",
+        "`define FOO1(BAR1, BAR2, BAR3) definition_body\n"
+        "`define FOO2(BAR1, BAR2, BAR3)\n",
+        N(0, L(0, {"`define", "FOO1", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", "definition_body"}))),
+        N(0, L(0, {"`define", "FOO2", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", ""}))),
+    },
+
+    {
+        "Partitioning formal arguments in consecutive `define's with body def",
+        "`define FOO1(BAR1, BAR2, BAR3)\n"
+        "`define FOO2(BAR1, BAR2, BAR3) definition_body\n",
+        N(0, L(0, {"`define", "FOO1", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", ""}))),
+        N(0, L(0, {"`define", "FOO2", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", "definition_body"}))),
+    },
+
+    {
+        "Partitioning formal arguments in consecutive `define's with body def",
+        "`define FOO1(BAR1, BAR2, BAR3) definition_body1\n"
+        "`define FOO2(BAR1, BAR2, BAR3) definition_body2\n",
+        N(0, L(0, {"`define", "FOO1", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", "definition_body1"}))),
+        N(0, L(0, {"`define", "FOO2", "("}),
+          N(2, L(2, {"BAR1", ","}), L(2, {"BAR2", ","}),
+            L(2, {"BAR3", ")", "definition_body2"}))),
+    },
+
+    {
+        "Partitioning formal argument with default value in `define",
+        "`define FOO(BAR1=default_val)\n",
+        N(0, L(0, {"`define", "FOO", "("}),
+          L(2, {"BAR1", "=", "default_val", ")", ""})),
+    },
+
+    {
+        "Partitioning formal arguments with default value in `define with "
+        "body definition",
+        "`define FOO(BAR1, BAR2=default_val) definition_body\n",
+        N(0, L(0, {"`define", "FOO", "("}),
+          N(2, L(2, {"BAR1", ","}),
+            L(2, {"BAR2", "=", "default_val", ")", "definition_body"}))),
+    },
+
     // TODO(fangism): decide/test/support indenting preprocessor directives
     // nested inside `ifdefs.  Should `define inside `ifdef be indented?
 };

diff --git a/verilog/parser/verilog.y b/verilog/parser/verilog.y
@@ -909,13 +909,13 @@ macro_formals_list
   : macro_formals_list ',' macro_formal_parameter
     { $$ = ExtendNode($1, $2, $3); }
   | macro_formal_parameter
-    { $$ = MakeNode($1); }
+    { $$ = MakeTaggedNode(N::kMacroFormalParameterList, $1); }
   ;
 macro_formal_parameter
   : PP_Identifier
-    { $$ = MakeNode($1); }
+    { $$ = MakeTaggedNode(N::kMacroFormalArg, $1); }
   | PP_Identifier '=' PP_default_text
-    { $$ = MakeNode($1, $2, $3); }
+    { $$ = MakeTaggedNode(N::kMacroFormalArg, $1, $2, $3); }
     /* $3 is unlexed text */
   ;