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.39k stars 214 forks source link

Reduce number of options for partition operations in verilog/formatting/tree_unwrapper.cc #181

Open fangism opened 4 years ago

fangism commented 4 years ago

This is what I would call a 'code-smell'.

Consider the function: https://github.com/google/verible/blob/2b217b603af444ecebc87c1277b5926c5f5232dc/verilog/formatting/tree_unwrapper.cc#L521

Abstractly, this is supposed to convert every subtree node of a CST into some form of token partition tree. Currently, here are example operations:

VisitNewUnindentedUnwrappedLine(node);
VisitNewUnwrappedLine(node);
VisitIndentedSection(...);
TraverseChildren(node);

This feels like too many choices, plus there is starting to creep some special case handling code that performs lower-level tree manipulations.

The final phase "post-traversal" fix-up starts here: https://github.com/google/verible/blob/2b217b603af444ecebc87c1277b5926c5f5232dc/verilog/formatting/tree_unwrapper.cc#L975

This introduces a few more high-level transformations:

FlattenOnce();
HostOnlyChild();
MergeConsecutiveSiblings();

plus more low-level tree manipulations.

I can't help but wonder, if this could be a lot cleaner if we more carefully limited the set of operations to choose from to a few high-level primitives.

What if we limited the subtree traversal phase to only VisitIndentedSection() (with different arguments in different cases)? Could we reduce the number of unique ways of handling node types? This would still accommodate +0-indent sections. This alone might result in deeper partition trees, but flattening and merging are cheap, and could be done in post-traversal, ideally using a limited vocabulary of tree-transforming primitive operations.

Outline would still resemble what we have today:

void TreeUnwrapper::Visit(const verible::SyntaxTreeNode& node) {
  // Construction phase: subtree traversal
  switch (node's tag) {
  }

  // Post-traversal shaping adjustment phase
  switch (node's tag) {
  }
}

I'm open to ideas for reducing complexity.

fangism commented 4 years ago

Major rewrite in progress.