Skip to content

Replace use of unsafeCoerce in freeze/thaw functions with discrete fo… #159

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
Oct 12, 2019

Conversation

andyarvanitis
Copy link
Contributor

…reign functions

The assumption that you can unsafeCoerce from an immutable to a mutable array and back doesn't hold for the golang backend – and maybe won't for other backends either, particularly ones that actually have immutable array types. Please let me know if you'd like any more info, or if you'd like me to create an issue for the change in addition to this PR. Thanks.

Reference: andyarvanitis/purescript-native-go-ffi#9

@hdgarrood
Copy link
Contributor

hdgarrood commented Sep 13, 2019

Hm, I'm not entirely sure about this: the docs specifically say that this is an O(1) operation, and I think there's a fair bit of code out there which assumes that STArray and Array have the same representation (or at least that the unsafe conversions are O(1)). In fact if they don't have the same representation there isn't really any reason for unsafeFreeze and unsafeThaw to exist at all.

@sharno
Copy link
Contributor

sharno commented Sep 13, 2019

The operation in JS and Go will still be O(1) for unsafe conversions, the problem is that Go discriminates between passing a parameter to a function by value (shallow copying) vs by pointer. This makes the usage of unsafeCoerce for both unsafeFreeze and unsafeThaw hard to handle in Go.

This is because unsafeThaw needs to return the pointer to deal with STArray as a pointer everywhere, while unsafeFreeze will return the the Array by value (copying only the pointer) everywhere it's passed to a function.

So unsafeFreeze and unsafeThaw will still have the advantage of being O(1) vs their safe counterparts which take O(n) to copy the whole content

@hdgarrood
Copy link
Contributor

Ah I see, in that case this ought to be fine. Just to check - when you say "shallow copying", does that mean that it is just the pointer address which is being copied, not the contents of the array?

@sharno
Copy link
Contributor

sharno commented Sep 13, 2019

Yeah I meant that only the pointer address is copied not the contents

@sharno
Copy link
Contributor

sharno commented Oct 10, 2019

@hdgarrood Is there any updates if this will be merged?

@hdgarrood hdgarrood merged commit 46f5a9c into purescript:master Oct 12, 2019
@andyarvanitis
Copy link
Contributor Author

Thanks, Harry!

@hdgarrood
Copy link
Contributor

Welcome :) would it help for me to cut a release?

@andyarvanitis
Copy link
Contributor Author

Sure, please, if you don't mind. I certainly don't have any dependencies on the current release.

@hdgarrood
Copy link
Contributor

👍 I've just published v5.3.1

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