Skip to content

Change ArrayBase.ptr to NonNull type #434

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

Closed
jturner314 opened this issue Mar 29, 2018 · 4 comments · Fixed by #683
Closed

Change ArrayBase.ptr to NonNull type #434

jturner314 opened this issue Mar 29, 2018 · 4 comments · Fixed by #683

Comments

@jturner314
Copy link
Member

Rust 1.25 introduces a NonNull type, which represents a pointer that must always be non-null. It would be nice to switch the ArrayBase.ptr field to type NonNull<S::Elem> to indicate that the pointer must always be non-null.

However, I'm not quite sure about the implications of NonNull<T> being "covariant over T". After reading this page in the nomicon, I think this is okay for Array, ArrayView, ArrayViewMut, and ArcArray, but I'm not sure that it's okay in general for arbitrary S: Data.

@bluss
Copy link
Member

bluss commented Apr 1, 2018

Sure, using NonNull should be good for us, don't see a problem with that. We need to remember to highlight the non-null requirement in ArrayView::from_shape_ptr. I'll have a think about if we already de facto require that pointer to be non-null (it certainly is non-null in all the safe ways of making an ArrayView).

@bluss
Copy link
Member

bluss commented Apr 1, 2018

NonNull was already listed over in #414, but I updated it to link to this issue as the specific one for that point.

@bluss
Copy link
Member

bluss commented Apr 1, 2018

(There is no “arbitrary S: Data” yet, because the Data trait is documented to be for our own purpose and we control all its implementations.)

@jturner314
Copy link
Member Author

There is no “arbitrary S: Data” yet

Specifically, I'm thinking about the raw pointer array views we discussed in #388, for which NonNull might not make sense. There may also be other Data types that we want to add in the future.

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 a pull request may close this issue.

2 participants