chromium / subspace

A concept-centered standard library for C++20, enabling safer and more reliable products and a more modern feel for C++ code.; Also home of Subdoc the code-documentation generator.
https://suslib.cc
Apache License 2.0
89 stars 15 forks source link

Add Iterator::try_collect #295

Closed danakj closed 1 year ago

danakj commented 1 year ago

https://github.com/rust-lang/rust/issues/94047

The open questions there are:

Should it have a more complicated signature to be able to return the partial results too? (@CAD97 https://internals.rust-lang.org/t/idea-fallible-iterator-mapping-with-try-map/15715/6?u=scottmcm )

No. That would defeat the primary purpose of making something more discoverable/accessible than collect() for results. If you need to do something more complicated you can use other tools for the job. Also consistency - the other try_ methods of iterators do not return partial results.

Should it take self rather than &mut self, to prevent users from accidentally continuing to use the iterator after a try_collect() failure? Note that you can still continue to use the iterator if you use by_ref() first, so it's not necessarily a functionality change.

No. Consistency with other try_ methods on iterators, and this makes it less useful. Fallible iteration is primarily useful to be able to terminate early for resuming after.

Does the name try_collect() conflict too much with the idea of collecting that's fallible in allocation? (i.e. collecting with Vec::try_reserve or similar)

The name is consistent with try_fold etc, which makes it discoverable. It should follow the same pattern for a short-circuiting fallible collect with try_collect.

CAD97 commented 1 year ago

(on the bit I've been quoted for)

try_collect : collect :: try_fold : fold, so I would say that try_collect should take &mut self, and that should be sufficient. If the successful part of the collect is desired, then a type PartialResult: FromIterator can be defined, with no need to make the try_collect signature itself more complicated.

This doesn't resolve the conflict between iterating results[^1] and fallible collection, but it keeps consistency with the other Iterator::try_* methods, and nobody is particularly enthused with the try_op pattern for fallible allocation APIs anyway.

[^1]: Iterating results is fn next(&mut self) -> Option<Result<_, _>>, fallible iteration is fn next(&mut self) -> Result<Option<_>, _> or perhaps more precisely -> enum { Next(_), Exhausted, Err(_) }. It's whether more elements may potentially be yielded after yielding an Err, or if an error indicates the exhaustion of the iterator and that another call to .next() may misbehave if the iterator is not fused.