-
Notifications
You must be signed in to change notification settings - Fork 32
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
Change list evaluation to collecting all pushed items on a new stack #2050
Conversation
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.
Make sure to add tests in InterpreterTests
I'll wait for user to have a check before merging |
val context = ctx.copy | ||
context.clear() |
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.
Maybe we could add a stack
parameter to Context#copy
to avoid having to clear later?
Can list elements still access what was on the stack before the list literal started executing? If not, maybe we could do something closer to the old code, except record the length of the stack before each element starts executing so we can pop the appropriate number of elements? But that would get messed up if you popped something that was on the stack already and pushed something else (e.g. Maybe if we could make a child context that only uses the parent context's when its own stack runs out of elements? But that's not how child contexts work right now |
@ysthakur lxyal and I agreed that allowing access to the outer stack leads to code that's hard to decide what should happen such as your example, so we opted for using a separate, empty-by-default stack. Maybe an alternative could be a parent stack that's read-only, so for example |
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.
Ah ok, makes sense then. LGTM
No description provided.