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

Avoid raw pointers in SliceBox and make the Send bound explicit #190

Merged
merged 1 commit into from
Jul 10, 2021
Merged

Avoid raw pointers in SliceBox and make the Send bound explicit #190

merged 1 commit into from
Jul 10, 2021

Conversation

adamreichold
Copy link
Member

@adamreichold adamreichold commented Jul 6, 2021

This is a follow-up to #189. I think the main benefit besides removing two unsafe usages is propagating the Send bound which seems to imply that Element must indeed be Send to be wrapped within SliceBox and managed by Python.

Taking this a bit further, there seems to be nothing slice or box specific about SliceBox, i.e. we could just as well have a

struct Opaque<T>(pub(crate) T);

which does not care about its contents at all, that is besides making it possible for Python to indirectly call T's Drop implementation.

@davidhewitt
Copy link
Member

making it possible for Python to indirectly call T's Drop implementation

This could also be done by using Python's capsule C API, which we could expose in PyO3. PyO3/pyo3#1193

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the change, since anyway all types we currently support as dtype require Send!

Taking this a bit further, there seems to be nothing slice or box specific about SliceBox, i.e. we could just as well have a

Yeah, but somehow NumPy's inheritance mechanism is very tricky and I'm not sure this is resuable.

@adamreichold adamreichold marked this pull request as ready for review July 6, 2021 07:47
@kngwyu kngwyu merged commit e261cb8 into PyO3:main Jul 10, 2021
@adamreichold adamreichold deleted the slice-box-safety branch July 11, 2021 08:53
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 this pull request may close these issues.

3 participants