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

Pin non-primitive function parameters #87

Merged
merged 8 commits into from
Jul 26, 2021
Merged

Pin non-primitive function parameters #87

merged 8 commits into from
Jul 26, 2021

Conversation

surma
Copy link
Collaborator

@surma surma commented Jul 20, 2021

Fixes #85

cc @RehkitzDev

@surma surma requested a review from torch2424 July 20, 2021 11:52
Copy link
Owner

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

LGTM! 😄 Thank you very much for the PR @surma ! And thanks for the feedback and help @dcodeIO and @MaxGraey ! 🙏🏾

Merging, and then I'll make a patch release! 😄

@@ -71,14 +71,25 @@ export function bindExportFunction(
`Expected ${argumentConverterFunctions.length} arguments, got ${args.length}`
);
}
const newArgs = args.map((v, i) =>
argumentConverterFunctions[i](
// The conversion function of the _next_ parameter could potentially
Copy link
Owner

Choose a reason for hiding this comment

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

Ah! Thank you for the comment here! It's super helpful! 😄 🎉

@@ -0,0 +1,8 @@
exports.mangleCompilerParams = params => {
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you very much for the tests as well! 😄 🎉

@torch2424
Copy link
Owner

@surma I can't seem to get Travis to be happy even though I did a whole new migration and things. 😭😭😭 Can you maybe sucking your local branch, closing this PR, and try opening a new PR and see if that works? 🤔

@torch2424
Copy link
Owner

(Closing and re-opening to maybe make travis happy?)

@torch2424 torch2424 closed this Jul 24, 2021
@torch2424 torch2424 reopened this Jul 24, 2021
@torch2424
Copy link
Owner

Finally found the travis issue: Could not authorize build request for torch2424/as-bind.

@torch2424
Copy link
Owner

Found the issue 🙃 travis-ci/docs-travis-ci-com#733 (comment)

@surma
Copy link
Collaborator Author

surma commented Jul 26, 2021

Thanks for landing this @torch2424! Appreciate you taking care of that part!

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.

Strings can get corrupted, due to them not being a Pinned GC Object
2 participants