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

A better Array::uninit and deprecate Array::uninitialized #902

Merged
merged 5 commits into from
Jan 30, 2021

Conversation

bluss
Copy link
Member

@bluss bluss commented Jan 30, 2021

Rename maybe_uninit to Array::uninit and move to base type

Old type situation: Array<MaybeUninit<i32>, D>::maybe_uninit() returns Array<MaybeUninit<i32>, D>

New type situation: Array<i32, D>::uninit() returns Array<MaybeUninit<i32>, D>

This is easier to use in generic code. There's also a new (internal) constructor build_uninit that makes
it possible to support any owned array for uninit initialization - for example in variants of map_collect.

Old Array::uninitialized is also deprecated. This is the first part of #804.

MSRV increases to 1.49 to be able to use the new definition of the DataOwned trait.

Old type situation: `Array<MaybeUninit<i32>, D>::maybe_uninit()`

New type situation: `Array<i32, D>::uninit()`

The link between the regular array storage type and the maybeuninit
version of it is made explicit in the DataOwned trait and this makes
it much easier to work with this constructor in generic code.

The new name is "uninit", just like many types in std (likej
`Box::uninit`).

The old name is deprecated. Because of the unfortunate generics
situation of the old name, the implementation is a copy & paste (short
allegory - inside ArrayBase::uninit we have types S and S::MaybeUninit
"available" as known type, inside ArrayBase::maybe_uninit we only have a
`DataOwned<Elem = MaybeUninit<_>>` type "avaialable" and no way to find the
corresponding "plain" storage type.)
This method allows creating any owned array and initializing it - the
method helps by giving mutable access even to normally copy-on-write
arrays, because when first created, the array is unshared.
This generalizes .map_collect() so that it supports any DataOwned array
storage. It is a crate internal method initially - it can be exported
later with more experience.
Array::uninitialized is very hard to use correctly (internally and for
our users). Prefer the new API with Array::uninit using `MaybeUninit<_>`
instead.
@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

@SparrowLii I have a commit here that shows my idea of how you'd use this to adapt your arithmetic ops changes for this https://github.com/rust-ndarray/ndarray/pull/new/bluss/pr-898-use-collect now that's a very messy commit, only one of the arithmetic ops impls is fully implemented, but it's meant to get the idea across without finishing it. Please don't merge master or this branch into your work (at least not until this PR is merged - I'd prefer rebasing after that, not merge).

@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

Rust 1.42 doesn't support the associated type and trait bound situation, trying to find which Rust fix or feature that we now depend on. Suspecting Rust 1.49 https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1490-2020-12-31

1.49 had a lot of fixes to associated types and bounds, and that enables
the new DataOwned changes to compile.
@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

Rust 1.49 confirmed by CI. It looks like this change rust-lang/rust#73905 was needed for DataOwned's new definition to compile.

@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

Breaking just by updating MSRV to 1.49

@bluss bluss added this to the 0.15.0 milestone Jan 30, 2021
@bluss bluss merged commit 04cc32d into master Jan 30, 2021
@bluss bluss deleted the better-uninit branch January 30, 2021 11:35
@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

cc #796 for deprecating uninitialized

@SparrowLii
Copy link
Contributor

@bluss Thank you very much for submitting this pr and it is amazing! I am trying to rebase my work to be able to use it

@jturner314
Copy link
Member

This new API is very nice. It's great to have a convenient, sound way for users to create uninitialized arrays and initialize them, while working cleanly with generic code.

@bluss
Copy link
Member Author

bluss commented Feb 1, 2021

So far build_uninit is internal - it could be made public (probably then using ArrayViewMut instead of a raw view) - I wrote we want som experience before making it public. But it's not necessary for users that work with Array only generic S: DataOwned

@SparrowLii
Copy link
Contributor

SparrowLii commented Feb 1, 2021

I think it’s better not to let users distinguish between A and MaybeUinit<A>. If users use uninit or build_uninit( if it becomes public), they will inevitably use unsafe code to call assume_init(). It feels a little strange for me.

@bluss
Copy link
Member Author

bluss commented Feb 1, 2021

Users can use our unsafe APIs, we need to enable that if nothing else for the expert use - personal projects and related crates that use ndarray. Uninitialized is deprecated because it's very hard to use, and we make a new try with the much improved MaybeUninit-based interface. It requires using unsafe code blocks, but not as much, and it should be easier to reason about.

If needed and possible to identify, we should add more safe constructors for covering common use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants