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 methods to collect Zip into an array #797

Merged
merged 7 commits into from
Apr 18, 2020
Merged

Add methods to collect Zip into an array #797

merged 7 commits into from
Apr 18, 2020

Conversation

bluss
Copy link
Member

@bluss bluss commented Apr 13, 2020

Add

  • Zip::apply_collect
  • Zip::apply_assign_into
  • Zip::par_apply_collect
  • Zip::par_apply_assign_into

Add internal (for now) constructors for MaybeUninit<T>-filled arrays.
Restrict to Copy elements to use collect (because we don't implement dropping a
halfway filled array yet).

This is added to all but the last arity of Zip (because the last item is needed for the result that we collect into.

The assignment functions are generalized so that we can assign to &mut T, &mut MaybeUninit<T> and &Cell<T> when we have elements of T. This seems powerful, and could be a concept that's explored further (methods .assign() and .fill() come to mind).
A very minor question is what to name and in which module to put the new trait for this - AssignElem<T>.

Fixes #755

src/zip/mod.rs Outdated Show resolved Hide resolved
@bluss
Copy link
Member Author

bluss commented Apr 13, 2020

In the grand scheme of things, it would be more exciting if

  1. any NdProducer could be collected
  2. Zip can be an NdProducer of tuples by default.
  3. One could with a method - .map(f) - create a different NdProducer from a Zip value itself.

but it's not certain that the required changes are worth it. Or if @jturner314 hasn't implemented something similar already :)

@bluss
Copy link
Member Author

bluss commented Apr 14, 2020

Now using MaybeUninit, because Vec::drop is what makes it unsound to toss uninit elements around. This is a lot closer to what @jturner314 said all along. 🙂

Use MaybeUninit to handle uninitialized data safely.
Use Array::maybe_uniit to not need to zero result elements; Restrict
to Copy, because we don't implement correct drop on panic yet.
@bluss bluss changed the title Add method Zip::apply_collect to collect results into Array Add methods to collect Zip into an array Apr 16, 2020
@bluss
Copy link
Member Author

bluss commented Apr 18, 2020

For non-Copy elements (more specifically, elements with drop, but we don't have a trait bound for that); there are hints of solutions for collect - I have an unbeautiful prototype for counting elements and dropping the right number of them on panic; but this doesn't transfer at all to the parallel case (in the parallel case, we have multiple different "write heads" into the array, and they all need to "roll back" if any thread panics), and that's a sign a better solution is needed of that problem. So it's not coming here.

I'm also saying that I think it's important to handle drop correctly - on panic, the elements that were created should drop. It would be easy to ignore the problem by just saying that there is no drop handling, but I would judge that's not robust.

That said, if MaybeUninit arrays are just exposed, they are very useful and the user can decide themselves if they want to skip handling drops on panic.

@bluss bluss merged commit 6fea7aa into master Apr 18, 2020
@bluss bluss deleted the zip-apply-collect branch April 18, 2020 11:35
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.

Is it possible to collect a Zip into Array?
1 participant