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

Why is MVector's constructor exposed? #517

Open
tomjaguarpaw opened this issue Dec 13, 2024 · 3 comments
Open

Why is MVector's constructor exposed? #517

tomjaguarpaw opened this issue Dec 13, 2024 · 3 comments

Comments

@tomjaguarpaw
Copy link
Member

The constructor of MVector is exposed:

data MVector s a  = MVector !Int !Int !(MutableArray s a)

This seems pretty odd, because the two Ints are the offset into the underlying MutableArray that the MVector starts, and the length of the MVector. both of which are used for bounds checking before deferring to readArray of a MutableArray via basicUnsafeRead.

So I'm confused. Firstly, can changing those fields not lead to segfaults? And secondly, doesn't changing those fields have no real purpose, since there are better ways of obtaining the ends one wants via the safe API (or even the unsafe one, with functions explicitly named as "unsafe" if one really wants the performance)?

Or have I missed something, and there is a good reason for exposing those fields?

@Shimuuar
Copy link
Contributor

It is unsafe, very unsafe. If you put wrong numbers there program may dance happy little jig all over the heap and then segfault horribly.

If you look at mutable storable and primitive vectors they expose constructors too. And it was like this since earliest versions. I think this was done in order to give immutable vectors access to internals of mutable ones. This is not particularly good reason but vector does poor job in separating safe and unsafe API

@tomjaguarpaw
Copy link
Member Author

OK, if the only reason is so that other modules can use the constructors then perhaps we can firstly place a big warning message next to them, and secondly, move them to a .Internal or .Unsafe module.

@Shimuuar
Copy link
Contributor

Documentation improvements are always welcome.

Moving constructors would be a breaking change and quite a bit of work. On top of it it's not that big improvement over current "just don't touch it and everything will be OK". It seems there's no one willing to do design and implementation work

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

No branches or pull requests

2 participants