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

Add as_ptr and as_mut_ptr methods #480

Merged
merged 2 commits into from Feb 3, 2019
Merged

Conversation

jturner314
Copy link
Contributor

@jturner314 jturner314 commented Nov 19, 2018

This is useful for moving across a matrix/slice in unsafe code and for converting slices to other types (e.g. ndarray::ArrayView).

Edit: If you'd like to guarantee that the pointer is aligned and non-null for empty matrices too, that would be great. I just didn't know if you'd like to make that guarantee, so I left it out.

This is useful for moving around a matrix/slice in unsafe code and for
converting slices to other types (e.g. `ndarray::ArrayView`).
@sebcrozet
Copy link
Member

Thanks!

If you'd like to guarantee that the pointer is aligned and non-null for empty matrices too, that would be great. I just didn't know if you'd like to make that guarantee, so I left it out.

How would this be done?

@jturner314
Copy link
Contributor Author

How would this be done?

This is a bit more difficult for nalgebra because nalgebra exposes so much of the internals compared to something like Vec or ndarray's ArrayBase. You'd want to require implementors of Storage to guarantee that their pointer is aligned and non-null. This would mean that types that implement Storage would have to control how they're constructed (e.g. prevent constructing empty storage from a null pointer), forbid public mutable access to their pointer, and maintain the aligned/non-null property in all methods. Essentially, Storage types need to maintain strict control of their pointer.

For comparison, consider how Vec maintains this invariant. It strictly controls how it's constructed, and constructors like new make sure to use an aligned and non-null pointer. Once the Vec is constructed, methods that modify the Vec maintain the invariant. (Methods don't really have to do anything to maintain this invariant; they just need to make sure not to do anything to violate it.) Finally, Vec doesn't allow users to directly modify its pointer; all modifications are performed using methods.

There is still a question of whether you'd like to publicly make this guarantee or not. Currently, ndarray's ArrayBase maintains the invariant that the ptr is non-null and aligned, but it doesn't publicly make this guarantee because we may want to relax the non-null constraint in the future. (See rust-ndarray/ndarray#543 for some recent discussion about this.)

@jturner314
Copy link
Contributor Author

By the way, I assumed that the pointer is always aligned for non-empty matrices, but I didn't check that thoroughly. (I assumed the nalgebra maintainers would know whether or not that's true.) It would be good to verify that's indeed the case or just remove that guarantee from the docs of .as_ptr()/.as_mut_ptr().

@jswrenn
Copy link
Sponsor Contributor

jswrenn commented Nov 25, 2018

It always struck me as a little odd that ptr is a method on Storage, instead of ContiguousStorage.

Having it be a method on Storage seems like it introduces the implicit requirement that it's possible to construct a pointer to the start of the storage space of the matrix, which would preclude implementing, for instance, a HashMapStorage.

@jturner314
Copy link
Contributor Author

Having it be a method on Storage seems like it introduces the implicit requirement that it's possible to construct a pointer to the start of the storage space of the matrix, which would preclude implementing, for instance, a HashMapStorage.

I assume you're thinking about a sparse representation with indices as keys and matrix elements as values? From what I can tell, Storage is restricted to accessing elements by offsetting a pointer. (For example, it has a strides method.) Note that getting a pointer and shape/strides is necessary for non-contiguous matrix types like MatrixSlice in addition to contiguous matrix types, and a trait like Storage allows operating in a uniform way on both contiguous and non-contiguous matrices. If sparse representations are desired in the future, I'd suggest adding a separate, more general trait.

@sebcrozet sebcrozet added enhancement P-medium Medium priority labels Dec 29, 2018
@sebcrozet sebcrozet added this to the v0.17 milestone Dec 29, 2018
@sebcrozet sebcrozet merged commit 24e6ce6 into dimforge:master Feb 3, 2019
@sebcrozet
Copy link
Member

Thanks @jturner314! I will merge this now for today's release.
The discussion about alignment is worth keeping in mind so I will create an issue for that.

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

Successfully merging this pull request may close these issues.

None yet

3 participants