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 for_each_mut method #910

Closed
wants to merge 5 commits into from

Conversation

jturner314
Copy link
Member

This PR does the following:

  • Rename the private unordered_foreach_mut method to for_each_mut.
  • Make for_each_mut public.
  • Unify some of the implementation of fold and for_each_mut.
  • Lengthen the lifetimes of the for_each_mut element borrows to match for_each. Unfortunately, the compiler rejects the previous implementation due to the longer borrows, so a compromise involving two checks for contiguity is used to avoid the compilation error.

It's unfortunate that the `.is_contiguous()` check is called
twice (once explicitly, and once in `.as_slice_memory_order_mut()).
The compiler rejects the following:

    if let Some(slc) = self.as_slice_memory_order_mut() {
        slc.iter_mut().for_each(f);
    } else {
        let mut v = self.view_mut();
        ...
    }

So, the chosen implementation is a safe compromise.
A: 'a,
S: DataMut,
{
if self.is_contiguous() {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid the double check, another version of the try_into_slice internal method would be needed (try_into_memory_order_slice or similar).

This whole method looks like it could be self.iter_mut().for_each(f) except that the iterator is order-preserving 🙁

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid the double check, another version of the try_into_slice internal method would be needed (try_into_memory_order_slice or similar).

Okay, I added a try_as_slice_memory_order_mut method to do this.

This whole method looks like it could be self.iter_mut().for_each(f) except that the iterator is order-preserving

Yeah, I've been thinking about the API of the iterators, .into_shape(), and other methods where there's a choice of order. One approach would be to introduce a wrapper type for specifying the order of things, similar to your builder suggestion, but applied before calling .iter()/.into_shape()/etc., rather than after. So, it would be used like this: arr.order(Order::Any).iter() for a "fast-order" iterator, or like this: arr.order(Order::Fortran).into_shape(new_shape) for reshaping with Fortran order.

Copy link
Member

Choose a reason for hiding this comment

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

try_as_slice_memory_order_mut is a nice solution

@bluss
Copy link
Member

bluss commented Feb 4, 2021

I just realized this method overlaps with the map_inplace method - simplest would be to keep the map_inplace method but give it the new lifetime powers, and for_each_mut is deleted? Then we have cleaned up the existing redundancy too.

@jturner314
Copy link
Member Author

I just realized this method overlaps with the map_inplace method

Oh, I missed that. Thanks for catching it. I've created #911 to replace this PR.

@jturner314 jturner314 closed this Feb 4, 2021
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.

None yet

2 participants