Skip to content

Mark Element unsafe #213

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.

Sign up for GitHub

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

Merged
kngwyu merged 2 commits into from
Nov 6, 2021
Merged

Mark Element unsafe #213

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebcd58b
Mark `Element` unsafe
kngwyu Nov 5, 2021
c581425
Revise the document for Element with using #Safety
kngwyu Nov 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/dtype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,19 @@ impl DataType {
}

/// Represents that a type can be an element of `PyArray`.
pub trait Element: Clone + Send {
///
/// Currently, only integer/float/complex types are supported.
/// If you come up with a nice implementation for some other types, we're happy to receive your PR :)
/// You may refer to the [numpy document](https://numpy.org/doc/stable/reference/c-api/dtype.html#enumerated-types)
/// for all types that numpy supports.
///
/// # Safety
///
/// A type `T` that implements this trait should be safe when managed in numpy array,
Copy link
Member

@adamreichold adamreichold Nov 5, 2021

Choose a reason for hiding this comment

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

I think adding the header

/// # Safety

before this section would be good so that Clippy's missing_safety_doc is appeased.

Copy link
Member Author

@kngwyu kngwyu Nov 6, 2021

Choose a reason for hiding this comment

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

Thanks! Could you please take a glance at the revised version?

Copy link
Member

@adamreichold adamreichold Nov 6, 2021

Choose a reason for hiding this comment

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

Looks good to me.

/// thus implementing this trait is marked unsafe.
/// For example, we don't support `PyObject` because of [an odd segfault](https://github.com/PyO3/rust-numpy/pull/143),
/// although numpy itself supports it.
pub unsafe trait Element: Clone + Send {
/// `DataType` corresponding to this type.
const DATA_TYPE: DataType;

Expand All @@ -191,7 +203,7 @@ pub trait Element: Clone + Send {

macro_rules! impl_num_element {
($t:ty, $npy_dat_t:ident $(,$npy_types: ident)+) => {
impl Element for $t {
unsafe impl Element for $t {
const DATA_TYPE: DataType = DataType::$npy_dat_t;
fn is_same_type(dtype: &PyArrayDescr) -> bool {
$(dtype.get_typenum() == NPY_TYPES::$npy_types as i32 ||)+ false
Expand Down