Skip to content

Add a read-only visitor pattern. #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

Pike
Copy link
Contributor

@Pike Pike commented Feb 7, 2019

This includes some performance tests to compare the visitor and a
traversal. Locally, the visitor is twice as fast.

This is the most basic Visitor pattern. I've had versions that were
more generic, and performance gains vanished quickly. In particular
optionally collecting results on exit halfed the performance benefits.

If we want to factor that in, we should have an independent base class
for that.

This should fix #68, I think.

@Pike Pike requested a review from stasm February 7, 2019 15:17
@Pike Pike added the fluent.syntax Issues related to the fluent.syntax module. label Feb 7, 2019
Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

This is great! Thanks so much, @Pike!

if not entering(node):
return
for propname, propvalue in vars(node).items():
self.visit(propvalue)
Copy link
Contributor

@stasm stasm Feb 7, 2019

Choose a reason for hiding this comment

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

I'd like to suggest a slightly more verbose way of writing this method. It's a nit, so feel free to ignore. As you know, I've been trying to promote the idea of self-documenting code in my reviews :) Here, the value I think this brings is that it gives an explicit meaning to the return value of the process_node method.

def visit_node(self, node):
    nodename = type(node).__name__
    process_node = getattr(self, 'visit_{}'.format(nodename), self.generic_visit)
    should_continue = process_node(node)
    if should_continue:
        for propname, propvalue in vars(node).items():
            self.visit(propvalue)

This includes some performance tests to compare the visitor and a
traversal. Locally, the visitor is twice as fast.

This is the most basic Visitor pattern. I've had versions that were
more generic, and performance gains vanished quickly. In particular
optionally collecting results on exit halfed the performance benefits.

If we want to factor that in, we should have an independent base class
for that.
@Pike Pike merged commit f3f9053 into projectfluent:master Feb 7, 2019
@Pike Pike deleted the BaseNode.visit branch February 7, 2019 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fluent.syntax Issues related to the fluent.syntax module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement BaseNode.visit API
2 participants