Change ArrayBase.ptr to NonNull type #683
Conversation
|
A fun very loose concern (I'm sorry, might just be interesting!) Rust has in the past had problems with loop optimization of iterators that yield Then the non-nullable annotation can get lost or confused in some optimization passes, and the result is that it's not always certain if null means the end of the loop or just the next pointer result of the iteration. When using NonNull inside Wait - but don't the most important iterators in Rust use We can just look out for it and check benchmarks |
|
Alright, I think I got it. Some benchmarks:
It looks like there might be a regression in taking dot products. |
|
Thanks a lot, I'd love to take a closer look at this. This is a good tool for benchmark comparisons: https://github.com/BurntSushi/cargo-benchcmp From master to the latest after unchecked: |
|
I've used the breaking-change label so that we remember to put a note in the relnotes, that raw pointers are now checked to be non-null and can never be null, even in empty array views (This has been true before too, but it's true now too 🙂). |
rawpointer 0.2 implements offset methods for NonNull<T>, which will be useful for us. This was already a dev-dependency, and also already a regular transitive dependency (from matrixmultiply).
Add two constructors that suffice for ndarray: - Get NonNull<T> from Vec<T>'s buffer pointer - Get NonNull<T> from raw pointer, and check if it's nonnull with a debug assertion In all other cases we can use .offset() on the NonNull (method from rawpointer 0.2, so we don't have to explicit roundtrip through raw pointers when we offset NonNull<T>. For safety, see this documentation in the rawpointer crate, which says why using .offset() on NonNull is safe. NonNull<T> supports the same offsetting methods under the same safety constraints as the other raw pointer implementations. There is no difference - both when offsetting *mut T and NonNull<T>, the offset is only well defined if we remain inside the same object or one-past the end, and we can never land in a null pointer while obeying those rules.
|
I've pushed two commits to the PR One adds dep on rawpointer 0.2 which has added an .offset() method for NonNull, which makes our code a lot simpler. (See commit log for more info). We could also copy the offset method into this crate, to reduce deps. Using .offset() simplifies a lot, and I factored out the two other way nonnull pointers were created. I hope this is ok. You've enabled maintainer pushes to the PR, and that's the reason I could update your branch. |
Bring in the latest changes and resolve conflicts (impl_views split)
|
I'm sorry for the mess. I've merged master and resolved conflicts, since another PR made this one conflict. (We could also rebase - please rebase and squash as needed if you want — I thought it was awkward to do that in a two-author branch). |
|
I think this is ready for merge, but it could be rebase/squashed if you want @berquist |
|
Whatever you would prefer. |
|
Great work on this! There are some places where it would be nice to replace more raw pointers with |
Closes #434
Should any of these
unwrapcalls turn into proper error handling? It looks like most functions already state that they'll panic if given an invalid pointer.It also looks like there's some wasteful
*mut A -> NonNull<A> -> *mut AorNonNull<A> -> *mut A -> NonNull<A>conversions happening (for example, inimpl_clone). What do you think about eliminating these?