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

Make the aview0, aview1, and aview2 free functions be const fns #1132

Merged
merged 5 commits into from
Mar 10, 2024

Conversation

jturner314
Copy link
Member

@jturner314 jturner314 commented Dec 7, 2021

This is something I've wanted for a long time; it allows for creation of const ArrayViews. With Rust 1.57, it's now possible. (This was blocked on the ability to panic in const fns.)

This PR also makes the Ix0, …, Ix6 free functions into const fns.

Unfortunately, the const fn implementations of aview0, aview1, and aview2 cannot use some existing internal functionality which would simplify the implementation, especially those which require the Dimension trait, since const traits / trait impls are not yet available. I think the proposed implementations are simple enough, though.

This will have a merge conflict with #1131. I'll fix it up to no longer conflict once #1131 is merged, assuming #1131 is merged first.

This is a breaking change in the sense that it requires Rust 1.57.

"Slice length must fit in `isize`.",
);
}
ArrayBase {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to use the internal builder methods, with_data_ptr etc? We'd prefer those instead of spreading out ArrayBase { .. } constructors again.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I try to make from_data_ptr into a const fn, then I get the following compilation errors:

error[E0658]: trait bounds other than `Sized` on const fn parameters are unstable
  --> src/impl_internal_constructors.rs:14:9
   |
14 | impl<A, S> ArrayBase<S, Ix1>
   |         ^
...
26 |     pub(crate) const unsafe fn from_data_ptr(data: S, ptr: NonNull<A>) -> Self {
   |     -------------------------------------------------------------------------- function declared as const here
   |
   = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information

error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
  --> src/impl_internal_constructors.rs:33:23
   |
33 |         debug_assert!(array.pointer_is_inbounds());
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0493]: destructors cannot be evaluated at compile-time
  --> src/impl_internal_constructors.rs:27:13
   |
27 |         let array = ArrayBase {
   |             ^^^^^ constant functions cannot evaluate destructors
...
35 |     }
   |     - value is dropped here

The biggest issue is the generic S type. I don't see a way to fix the errors while keeping a generic S. We could add const constructors to impl_internal_constructors.rs specifically for ArrayView, though.

@bluss
Copy link
Member

bluss commented Dec 7, 2021

ArrayRef is hopefully coming in 0.16. I don't know if these conflict, but I suggest that these changes have to stand back if they can't be realized in the ArrayRef update.

@jturner314
Copy link
Member Author

ArrayRef is hopefully coming in 0.16. I don't know if these conflict, but I suggest that these changes have to stand back if they can't be realized in the ArrayRef update.

I don't see any reason why they'd conflict, but if you'd feel more comfortable with waiting until later to do this, that would be fine with me. The only breaking change in this PR is the minimum Rust version, so it would be good to at least update the MSRV in the next breaking release, unless you consider Rust 1.57 to be too new.

@bluss bluss added this to the 0.16.0 milestone Mar 10, 2024
@bluss bluss force-pushed the const-constructors branch 2 times, most recently from 99d6ce3 to caf4688 Compare March 10, 2024 08:50
@bluss
Copy link
Member

bluss commented Mar 10, 2024

There are already breaking changes in the master branch (#980), so continue with this here. Thanks for this one.

@bluss bluss added this pull request to the merge queue Mar 10, 2024
Merged via the queue into rust-ndarray:master with commit cd0a956 Mar 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants