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

fix for issue #1634 -- for passing a closure to exported Go function #1646

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

rcoreilly
Copy link
Contributor

This fixes issue #1634

includes special case for defer function.

I could remove or significantly reduce the comment description, and just have that here for future reference:

// per #1634, if v is already a func, then don't re-wrap! critically, the original wrapping
// clones the frame, whereas the one here (below) does not clone the frame, so it doesn't
// generate the proper closure capture effects!
// this path is the same as genValueAsFunctionWrapper which is the path taken above if
// the value has an associated node, which happens when you do f := func() ..

Copy link
Member

@mvertes mvertes left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this bug.

Ok to me, once the linter error on test case is fixed.

@mvertes mvertes self-requested a review July 19, 2024 16:48
Copy link
Member

@mvertes mvertes left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker
Copy link
Contributor

🚫 failed to update: update branch: PUT https://api.github.com/repos/traefik/yaegi/pulls/1646/update-branch: 403 user doesn't have permission to update head repository []

@mvertes
Copy link
Member

mvertes commented Jul 19, 2024

@rcoreilly if it is ok for you, allowing 'edit by maintainers' on next pull requests would permit us to better automate rebase at merge (when no conflicts) and manage not up-to-date branches.

In the meantime, you have to update the branch yourself, so we can proceed.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants