Skip to content
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

Populate and use ObjFn->arity correctly (#731) #1124

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mhermier
Copy link
Contributor

ObjFn->arity was only populated for functions. Fix that with wren call handles, and regular methods, so the stack can be properly adjusted when invoking wrenCallFunction.

`ObjFn->arity` was only populated for functions. Fix that with
wren call handles, and regular methods, so the stack can be properly
adjusted when invoking `wrenCallFunction`.

wrenCallFunction(vm, vm->fiber, closure, 0);
wrenCallFunction(vm, vm->fiber, closure, closureNumArgs);
Copy link
Member

Choose a reason for hiding this comment

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

this code might not behave as expected? The closure being called here is the stub function created by the call handle, which only has bytecode to forward the call to the receiver. The closure itself has 0 args marked here specifically to avoid interacting with the stack afaict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old behavior was ONLY correct when passing the correct number of arguments using broken logic. There was a bogus relation between what the last wrenCallFunction argument was intended to be and what it was really used for. With that change, the argument is now the number of arguments that are on the stack to perform the execution call. Because of that it is not 0 anymore, since obviously a closure can always have any number arguments.
Because a closure has to accept its arity or more arguments, as per Fn.call, there MUST be a stack interaction if there is more arguments than required.

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.

None yet

2 participants