Add new method .assign_to() #947
Conversation
|
Unsure if these methods pull their weight (they add nothing new, but are expressive because they are easy to use). Their properties contrast with the existing Best we can do for existing .assign is to generalize it, with some loss of type inference, so that it can assign to both "T" and "ManuallyUninit" arrays. The assign_into solution additionally handles "Cell" arrays. |
bluss
force-pushed
the
assign-into
branch
2 times, most recently
from
98970f5
to
b22e2b1
Compare
bluss
changed the title
src/impl_methods.rs
Outdated
| /// [`AssignElem`] determines how elements are assigned. | ||
| /// | ||
| /// **Panics** if shapes disagree. | ||
| pub fn map_assign_into<'a, F, B, P>(&'a self, f: F, to: P) |
There was a problem hiding this comment.
Maybe skip exposing this as a public method, it's not as used.
There was a problem hiding this comment.
Yeah, I'm not sure this method is necessary. The parameters are also a little awkward, because while I think the order f: F, to: P better fits the name of the method, it's more common for Rust methods to put the closure as the last parameter since closures are often formatted as multiple lines. (Consider e.g. Result::map_or, which IMO would better fit the name as map_or<F, U>(self, f: F, default: U) but is actually map_or<U, F>(self, default: U, f: F) in the standard library.)
There was a problem hiding this comment.
I do think it's worth having a simple way to assign to arrays of MaybeUninit, although as you pointed out, this could be handled with the existing assign. I'm not opposed to an assign_to method, though, because it's a simple addition, and the swapped parameter order could be useful in some cases. The fact that it works with arrays of Cell/MathCell is a bonus, although I'm unclear on the typical use cases for arrays of Cell/MathCell.
src/impl_methods.rs
Outdated
| /// [`AssignElem`] determines how elements are assigned. | ||
| /// | ||
| /// **Panics** if shapes disagree. | ||
| pub fn assign_into<P>(&self, to: P) |
There was a problem hiding this comment.
I think a better name would be assign_to, because this method takes &self rather than self.
There was a problem hiding this comment.
Yes, I guess it's better to stick to that convention, even though it's not always clear cut. Consuming a producer seems to be a very "into" situation, but in most cases, it will just be a mutable reference to an array or view.
src/impl_methods.rs
Outdated
| /// [`AssignElem`] determines how elements are assigned. | ||
| /// | ||
| /// **Panics** if shapes disagree. | ||
| pub fn map_assign_into<'a, F, B, P>(&'a self, f: F, to: P) |
There was a problem hiding this comment.
Yeah, I'm not sure this method is necessary. The parameters are also a little awkward, because while I think the order f: F, to: P better fits the name of the method, it's more common for Rust methods to put the closure as the last parameter since closures are often formatted as multiple lines. (Consider e.g. Result::map_or, which IMO would better fit the name as map_or<F, U>(self, f: F, default: U) but is actually map_or<U, F>(self, default: U, f: F) in the standard library.)
This method does the same job as Zip::from(a).map_assign_into(f, b) with cloning as the function f. Comparison with existing assign: - Reverse argument order: source, destination - The new method doesn't broadcast. - The new method is generic over AssignElem (assign into &Cell, &mut ManuallyUninit etc).
bluss
changed the title
This method does the same job as Zip::from(a).map_assign_into(f, b) with
cloning as the function f.
Comparison with existing assign:
ManuallyUninit etc).