chipsalliance / verible

Verible is a suite of SystemVerilog developer tools, including a parser, style-linter, formatter and language server
https://chipsalliance.github.io/verible/
Other
1.38k stars 214 forks source link

module with exported function split across too many lines #496

Closed fangism closed 3 years ago

fangism commented 4 years ago

Test case

module m;
  export "DPI-C" function mhpmcounter_get;
endmodule

Actual output

module m;
  export "DPI-C"
  function mhpmcounter_get
  ;
endmodule

Expected or suggested output

Original form looked better.

fangism commented 4 years ago
$ verible-verilog-format --show_token_partition_tree ~/testing/verilog/formatting/export.sv
Full token partition tree:
{ ([<auto>], policy: always-expand) @{}
  { ([<auto>], policy: always-expand) @{0}, (origin: "module m;
 ...  endmodule")
    { ([module m ;], policy: fit-else-expand, (origin: "module m;")) }
    { (>>[<auto>], policy: tabular-alignment) @{0,1}, (origin: "export "DPI...ounter_get;")
      { (>>[export "DPI-C"], policy: fit-else-expand) }
      { (>>[function mhpmcounter_get], policy: fit-else-expand, (origin: "function mhpmcounter_get")) }
      { (>>[;], policy: uninitialized) }
    }
    { ([endmodule], policy: always-expand, (origin: "endmodule")) }
  }
}

This is a token partitioning problem.

fangism commented 4 years ago

Particularly strange is the fact that the entire export declaration node was marked for tabular-alignment.

fangism commented 4 years ago

b/169255663

fangism commented 4 years ago

The tabular-alignment designation came from NodeEnum::kModuleItemList because module-items can contain alignable sections, but is not necessarily alignable in its entirety. When the export node got hoisted/flattened, it inadvertently turned a non-alignment partition into an align-handled one.

We need to prevent that in tree_unwrapper.cc during partition reshaping.

fangism commented 4 years ago

The function that needs updating is ReshapeTokenPartition.