-
Notifications
You must be signed in to change notification settings - Fork 507
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
[WIP] Add try_fold, try_for_each, try_reduce #563
Conversation
I think the proper behavior of
The alternative being proposed is that, once fold has settled on a sequential list of things to process, it will always finish, and hence you can very easily get multiple Given these two options, I suspect that we want the current behavior. It seems to me that the are only reason to want the second behavior is if you somehow wanted to ensure that some elements were processed even though a The only property that you do get, afaict, is that if a In contrast, what we give up is that we potentially do a bunch of extra work, producing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from needing tests/docs/etc, I feel pretty good about mirroring the std API. However, we definitely want to keep the Try
trait private (as indeed we have done here).
I'm proposing that I don't know if there's an actual useful semantic here, beyond avoiding redundant |
I guess what you thought I was proposing was that all sequential runs would be processed up to an |
src/iter/try_reduce.rs
Outdated
} | ||
|
||
fn full(&self) -> bool { | ||
self.result.is_err() || self.full.load(Ordering::Relaxed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't self.result.is_err()
impliy self.full
is true? (Or will be shortly?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. In theory the local check could be faster than an atomic load, even relaxed, but I suppose that's micro-optimizing for the uncommon error case. I'll just check the latter.
It will now only consider the locally-folded items for short-circuit behavior. It's expected that this will commonly be followed by a `try_reduce` or the like with its own global effect.
Even though we already know `Self::Item: Send`, this doesn't apply constraints to its `Try::Ok` and `Error`. Therefore, if we map to `Result<Option<Ok>, Error>` at a high level, we have to require extra `Send` bounds to make sure that map is `Send`, like: <Self::Item as Try>::Ok: Send, <Self::Item as Try>::Error: Send We can avoid this by implementing a custom `Consumer`, where we only hold it as a `Result` in the low level `Folder` regardless of `Send`.
bors r+ |
563: [WIP] Add try_fold, try_for_each, try_reduce r=nikomatsakis a=cuviper There are six variations here, matching the existing non-try suite: - `try_fold` and `try_fold_with` - `try_for_each` and `try_for_each_with` - `try_reduce` and `try_reduce_with` All of them operate on `Try::Ok` values similar to the exiting non-try methods, and short-circuit early to return any `Try::Error` value seen. This `Try` is a pub-in-private clone of the unstable `std::ops::Try`, implemented for `Option<T>` and `Result<T, E>`. TODO and open questions: - [ ] Needs documentation, examples, and tests. - [x] Should we wait for `Iterator::try_fold` and `try_for_each` to reach rust stable? They were stabilized in rust-lang/rust#49607, but there's always a chance this could get backed out. - **Resolved**: they're stable in 1.27 - [x] Should we wait for stable `std::ops::Try`? We could just keep ours private for now, and change to publicly use the standard trait later (on sufficiently new rustc). - **Resolved**: keep our pub-in-private `Try` for now. - [x] Should `try_fold` and `try_fold_with` continue to short-circuit globally, or change to only a local check? - When I first implemented `try_reduce_with`, I use a `try_fold` + `try_reduce` combination, like `reduce_with`'s implementation, but I didn't like the idea of having double `full: AtomicBool` flags in use. - If `try_fold` only errors locally, then other threads can continue folding normally, and you can decide what to do with the error when you further reduce/collect/etc. e.g. A following `try_reduce` will still short-circuit globally. - **Resolved**: changed to just a local check. Closes #495. Co-authored-by: Josh Stone <cuviper@gmail.com>
There are six variations here, matching the existing non-try suite:
try_fold
andtry_fold_with
try_for_each
andtry_for_each_with
try_reduce
andtry_reduce_with
All of them operate on
Try::Ok
values similar to the exiting non-trymethods, and short-circuit early to return any
Try::Error
value seen.This
Try
is a pub-in-private clone of the unstablestd::ops::Try
,implemented for
Option<T>
andResult<T, E>
.TODO and open questions:
Iterator::try_fold
andtry_for_each
toreach rust stable? They were stabilized in Stabilize iterator methods in 1.27 rust-lang/rust#49607,
but there's always a chance this could get backed out.
std::ops::Try
? We could just keepours private for now, and change to publicly use the standard
trait later (on sufficiently new rustc).
Try
for now.try_fold
andtry_fold_with
continue to short-circuitglobally, or change to only a local check?
try_reduce_with
, I use atry_fold
+try_reduce
combination, likereduce_with
's implementation, butI didn't like the idea of having double
full: AtomicBool
flagsin use.
try_fold
only errors locally, then other threads can continuefolding normally, and you can decide what to do with the error
when you further reduce/collect/etc. e.g. A following
try_reduce
will still short-circuit globally.
Closes #495.