Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feature[next]: Non-tree-size-increasing collapse tuple on ifs #1762
feature[next]: Non-tree-size-increasing collapse tuple on ifs #1762
Changes from 15 commits
34d6040
48abc08
3790944
a8a63bf
2c44ffc
42b5817
9da19a2
bcd9e48
0a212bd
70562fe
9cee650
43f5741
7b37f1c
a0341a6
5a892f3
914a9e5
fc46edf
4e12195
f8b5b99
5e1a88c
53d442a
3b1af1e
96a840a
9263f4d
4d700e7
72ca50d
2a0bad1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this assert useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case one (erronously) returns the same node again instead of
None
to signify no change,fp_transform
runs into an endless loop. This is a cheap way to prevent this case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we iterate over any functions args or do we know more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't know more. We need to build and simplify the continuation first to know if we want to transform. It might be possible to handle multiple ifs, but the code would become more complex and error prone, and there is no benefit. It is just important to remember that the entire node is split into the continuation and the first matching
if
argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how it looks, but maybe do the same here:
if not cpm.is_call_to(arg, "if_"): continue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the missing type from a previous transform in the fp iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as discussed the approach is reinfer on demand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under which conditions is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this recursion, we handle all the other args after the current one? I am confused...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expression is split into a continuation (which includes all the args following the current one) and the currently transformed arg. Since the currently used arg will use the newly build continuation it is natural to first build & transform the continuation and only then tackle the arg. In the new node (continuation + arg) the continuation appears first. So in the expression the continuation appears first, but evaluation is still first arg then continuation. This is somewhat what continuations are all about (https://en.wikipedia.org/wiki/Continuation-passing_style):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically here it's decided that we actually do something.