Skip to content

Conversation

jturner314
Copy link
Member

I'd like to make CowRepr implement DataOwned, where DataOwned is changed to mean "a storage type which can own its data". The only issue is the .into_shared() method, which says, "Turn the array into a shared ownership (copy on write) array, without any copying." We can't always turn a CowArray into an ArcArray without copying. One approach (which would be a breaking change) would be to remove DataOwned::into_shared and just rely on this From implementation instead. Then, we could implement DataOwned for CowRepr without issues.

Regardless of the CowRepr stuff, this impl still seems useful.

@bluss bluss added this to the 0.15.3 milestone Jun 5, 2021
@bluss
Copy link
Member

bluss commented Jun 5, 2021

Agree. I hope to keep the into_shared etc methods somehow, but it's clear that they need to change to support wider diversity of owned arrays (including future differences in allocator). Maybe adding a Clone bound to into_shared() would be ok and sufficient, and then the specific From impls can be used by users when they need to avoid Clone requirements.

@bluss bluss merged commit f5c18a5 into rust-ndarray:master Jun 5, 2021
@jturner314
Copy link
Member Author

Maybe adding a Clone bound to into_shared() would be ok and sufficient, and then the specific From impls can be used by users when they need to avoid Clone requirements.

Yeah, I think this makes the most sense. We'd move into_shared from DataOwned to Data, add an A: Clone bound, and drop the "without any copying" guarantee. In most cases, the user would just use .into_shared() to convert arrays to ArcArray (so that all they'd need is are A: Clone and S: Data bounds), but if they want to create an ArcArray from an array without A: Clone, they would use an Into<ArcArray> bound instead.

@jturner314 jturner314 deleted the impl-from-Array-for-ArcArray branch June 5, 2021 20:40
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