Skip to content

fix stack issues with AST_Node.evaluate()#1603

Merged
alexlamsl merged 2 commits intomishoo:masterfrom
alexlamsl:evaluate
Mar 15, 2017
Merged

fix stack issues with AST_Node.evaluate()#1603
alexlamsl merged 2 commits intomishoo:masterfrom
alexlamsl:evaluate

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

As patched in #1597, make_node_from_constant() makes inconsistent and sometimes incorrect calls to optimize() and transform().

Fix those issues properly by changing the semantics of evaluate() and make_node_from_constant(), with the side effect that evaluate() no longer eagerly converts constant to AST_Node.

As patched in mishoo#1597, `make_node_from_constant()` makes inconsistent and sometimes incorrect calls to `optimize()` and `transform()`.

Fix those issues properly by changing the semantics of `evaluate()` and `make_node_from_constant()`, with the side effect that `evaluate()` no longer eagerly converts constant to `AST_Node`.
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc as per 8223b2e#commitcomment-21332046, PTAL at c2f6bc4 and see if you want to reword it.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 15, 2017

The comment is helpful. Thanks. But I think the comment should be above both descend()s and also explain the more basic question why it is called twice in a row. Why is one pass insufficient and does this impact performance?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc that is the million dollar question 😏

... what I mean is, in #1593 when I make everything work in the simplest way possible, it harms overall performance significantly. So I decided to attack this from a different angle - preserve existing steps as much as possible whilst incrementally fix up stack-related issues one by one.

So I struggle to describe what this does other than "it did so before and it worked", at least for now. 😓

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 15, 2017

If the code in question is changed to call descend() just once - will it produce incorrect code, or just suboptimal?

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 15, 2017

Even a comment to the effect that two calls to descend() are needed to produce more optimal code would be sufficient.

Edit: while keeping your existing explanation.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc force-pushed revised comment as 200090a

Thanks for the nudge to write this comment out btw, even though I'm struggling with it 😅

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 15, 2017

The comments are good. They'll serve as a warning to anyone looking to modify this code.

@alexlamsl alexlamsl merged commit cf4bf4c into mishoo:master Mar 15, 2017
@alexlamsl alexlamsl deleted the evaluate branch March 15, 2017 17:03
@alexlamsl alexlamsl mentioned this pull request Mar 17, 2017
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.

2 participants