Skip to content

Implement Iterable for ast.Node instead of a hand-crafted Traversable#497

Merged
tdardinier merged 1 commit into
masterfrom
add-iterable
Feb 15, 2021
Merged

Implement Iterable for ast.Node instead of a hand-crafted Traversable#497
tdardinier merged 1 commit into
masterfrom
add-iterable

Conversation

@fpoli

@fpoli fpoli commented Feb 12, 2021

Copy link
Copy Markdown
Member

As @marcoeilers pointed out, PR #493 removed too many methods from the interface. Manually re-implementing all of them in the utils.Traversable trait is a lot of work. This PR provides a different trade-off between performance and usability.

Pro:

  • ast.Node extends again the Iterable trait, so it inherits all its convenient methods.
  • foreach just calls the visitor and is thus efficient.
  • Compared to 1df0f34, iterator's implementation just allocates one collection containing all the AST's subnodes, rather than doing it for each node of the AST.

Cons:

  • All methods inherited from ast.Node will internally call iterator, thus they'll create a collection containing all the AST's subnodes. In most of the cases, the allocation can be avoided by overriding the method such that it internally uses foreach instead of iterator.

@fpoli fpoli self-assigned this Feb 12, 2021
@fpoli fpoli force-pushed the add-iterable branch 3 times, most recently from 7277d52 to c037504 Compare February 12, 2021 15:11
trait Node extends Iterable[Node] with Rewritable {

/** @see [[Nodes.subnodes()]] */
def subnodes = Nodes.subnodes(this)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fpoli If we bake in the assumption that all nodes are immutable, we could turn subnodes and iterator (and maybe other defs) into lazy vals. This could speed up subsequent traversals of the same tree.

@tdardinier tdardinier merged commit 4446989 into master Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants