Skip to content

Remove the TRAVERSE_CALLS option in the ConstantExpressionRunner #6449

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
Mar 29, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented Mar 28, 2024

The implementation of calls with this option was incorrect because it cleared
the locals before evaluating the call arguments. The likely explanation for why
this was never noticed is that there are no users of this option, especially
since it is exposed in the C and JS APIs but not used internally.

Rather than try to fix the implementation, just remove the option.

The implementation of calls with this option was incorrect because it cleared
the locals before evaluating the call arguments. The likely explanation for why
this was never noticed is that there are no users of this option, especially
since it is exposed in the C and JS APIs but not used internally.

Rather than try to fix the implementation, just remove the option.
@tlively tlively requested a review from kripken March 28, 2024 03:35
@tlively
Copy link
Member Author

tlively commented Mar 28, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite Graphite

@kripken
Copy link
Member

kripken commented Mar 29, 2024

Looks like this was added in 483d759 (#2702) by @dcodeIO - @dcodeIO is this in use?

@dcodeIO
Copy link
Contributor

dcodeIO commented Mar 29, 2024

Thanks for the heads up! It appears that we are no longer using this option.

@tlively tlively merged commit b10d59d into main Mar 29, 2024
@tlively tlively deleted the remove-traverse-calls branch March 29, 2024 20:01
@gkdn gkdn mentioned this pull request Aug 31, 2024
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