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

Simplify s![] #1195

Closed
wants to merge 2 commits into from
Closed

Simplify s![] #1195

wants to merge 2 commits into from

Conversation

CAD97
Copy link

@CAD97 CAD97 commented Aug 13, 2022

Fixes #1194 with a very slight generalization to make this arm more similar to the other arms.

src/slice.rs Outdated
::core::marker::PhantomData::<$crate::Ix0>,
::core::marker::PhantomData::<$crate::Ix0>,
)
$crate::SliceInfo::new_unchecked([], $in_dim, $out_dim)
Copy link
Member

Choose a reason for hiding this comment

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

what's the impact on the unsafe code block here?

Copy link
Author

@CAD97 CAD97 Aug 13, 2022

Choose a reason for hiding this comment

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

It's still necessary, as SliceInfo::new_unchecked is unsafe.

Technically, this means that someone using the macro could write

s! { @parse
    {
        // statements in an unsafe context
        ::core::marker::PhantomData::<::ndarray::Ix0>
    },
    ::core::marker::PhantomData<::ndaray::Ix0>,
    []
}

to get an unsafe context without writing unsafe, but this is an internal rule, so that's not too worrisome. However, it is reasonable to evaluate the arguments outside of the unsafe block just for thoroughness.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for bringing this up. Some of the other @parse arms had safety issues if called directly. I've created #1196 to fix that. With that change, we can change this PR to modify the "empty case" arm to the following:

    // empty call, i.e. `s![]`
    (@parse $in_dim:expr, $out_dim:expr, []) => {
        {
            debug_assert_eq!($in_dim, ::core::marker::PhantomData::<$crate::Ix0>);
            debug_assert_eq!($out_dim, ::core::marker::PhantomData::<$crate::Ix0>);
            (
                [],
                ::core::marker::PhantomData::<$crate::Ix0>,
                ::core::marker::PhantomData::<$crate::Ix0>,
            )
        }
    };

This is safe, since it always expands to valid values for the SliceInfo::new_unchecked call (which is in the ($($t:tt)*) arm with #1196). It has debug assertions to ensure that $in_dim and $out_dim are the expected values. (They'll always be the expected values, unless the user calls s![@parse ...] directly.

@CAD97
Copy link
Author

CAD97 commented Aug 13, 2022

Check failures are

  • unrelated (new clippy warning)
  • spurious docker error?

    docker: Error response from daemon: Duplicate mount point: /home/runner/work/ndarray/ndarray.

@bluss
Copy link
Member

bluss commented Mar 10, 2024

I'm sorry this has now been conflicted by other changes, so I'll close this for now. I'm not sure about the simplification, the old version makes sure the dimensions are really Ix0 (empty) for the empty slice, and this one doesn't have the same effect? Not sure.

@bluss bluss closed this Mar 10, 2024
@CAD97 CAD97 deleted the patch-1 branch March 10, 2024 13:17
@CAD97
Copy link
Author

CAD97 commented Mar 10, 2024

It doesn't, tbf. The main reason I made the PR is that $crate in macro patterns is pretty cursed.

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.

Use of $crate in macro patterns is questionable
3 participants