Fix to_shared to avoid cloning when it is already ArcArray #893
Conversation
src/data_traits.rs
Outdated
| @@ -96,6 +96,15 @@ pub unsafe trait Data: RawData { | |||
| where | |||
| Self::Elem: Clone, | |||
| D: Dimension; | |||
|
|
|||
| fn to_share<D>(self_: &ArrayBase<Self, D>) -> ArrayBase<OwnedArcRepr<Self::Elem>, D> | |||
There was a problem hiding this comment.
Very nice, let's name this method to_shared IMO
There was a problem hiding this comment.
Okay, they should be consistent
src/data_traits.rs
Outdated
| D: Dimension, | ||
| { | ||
| // to shared using clone of OwnedArcRepr without clone of raw data. | ||
| ArrayBase { |
There was a problem hiding this comment.
Can't this method body just be self_.clone()? We should avoid writing out the ArrayBase constructor just to keep maintainability.
There was a problem hiding this comment.
That's right. Thanks for pointing that out.
src/data_traits.rs
Outdated
| @@ -96,6 +96,15 @@ pub unsafe trait Data: RawData { | |||
| where | |||
| Self::Elem: Clone, | |||
| D: Dimension; | |||
|
|
|||
| fn to_share<D>(self_: &ArrayBase<Self, D>) -> ArrayBase<OwnedArcRepr<Self::Elem>, D> | |||
There was a problem hiding this comment.
| fn to_share<D>(self_: &ArrayBase<Self, D>) -> ArrayBase<OwnedArcRepr<Self::Elem>, D> | |
| #[doc(hidden)] | |
| fn to_shared<D>(self_: &ArrayBase<Self, D>) -> ArrayBase<OwnedArcRepr<Self::Elem>, D> |
Like the rest of the methods. And it should have a doc comment here still - as documentation.
There was a problem hiding this comment.
Yes, the corresponding modifications have been added.
|
I prefer merging with a merge commit, makes it clear what every unit of contribution has been. This PR contains a commit called "back consist" which seems messy. I'll prefer to squash merge so that that one is not visible. |
|
Thanks for this fix! |
That is my mistake. I made some unnecessary changes on my master branch, so I used reset to eliminate it. Squash merge is all right. |
From FIXME in to_shared. The current implementation simply calls to_owned().into_shared(), so cloning cannot be avoided when the array is already ArcArray.
So use to_owned().shared() in a method of Data trait (just like into_owned()) and add a default implementation to it, which will not affect the existing structure. Then modify its implementation in OwnedArcRepr, which implements the Data trait, to avoid cloning when the array is already ArcArray.