Implement FuturesUnordered::iter_mut #618

srijs · 2017-10-22T03:40:46Z

Implementing FuturesUnordered::iter_mut as suggested in #605. Generally it should be said that I don't have a lot of experience with unsafe rust yet, so I would appreciate a thorough review!

I did not go for the other suggested ones (into_iter/drain), since I lack the insight into how to implement this safely without leaking memory.

carllerche

The impl looks correct to me. I added some naming related comments.

carllerche · 2017-10-23T16:40:13Z

src/stream/futures_unordered.rs

@@ -427,6 +436,37 @@ impl<T> Drop for FuturesUnordered<T> {
    }
 }

+#[derive(Debug)]
+/// Mutable iterator over all futures in the unordered set.
+pub struct FuturesUnorderedIterMut<'a, F: 'a> {


I would probably just name this IterMut as it already is in the futures_unordered module.

carllerche · 2017-10-23T16:40:43Z

src/stream/mod.rs

@@ -112,7 +112,7 @@ if_std! {
    pub use self::collect::Collect;
    pub use self::wait::Wait;
    pub use self::split::{SplitStream, SplitSink};
-    pub use self::futures_unordered::{futures_unordered, FuturesUnordered};
+    pub use self::futures_unordered::{futures_unordered, FuturesUnordered, FuturesUnorderedIterMut};


I probably wouldn't re-export this type. Instead, maybe make the futures_unordered module pub (and ensure no priv types are accidentally exported).

srijs · 2017-10-24T08:54:39Z

@carllerche Thanks for the review, addressed all feedback!

alexcrichton · 2017-10-30T15:48:52Z

Inside this module there's a public futures_unordered function which if we make the module public will also make that function reachable (but it's not intentional to have that function reachable. I think I'm ok making the module public for the iterator name (albeit it's a bit of a wonky setup) but can we ensure that the free function isn't exposed?

alexcrichton · 2017-10-30T15:51:06Z

src/stream/futures_unordered.rs

+pub struct IterMut<'a, F: 'a> {
+    node: *const Node<F>,
+    len: usize,
+    _marker: PhantomData<&'a F>


I think this may best be &'a mut FuturesUnordered<F> perhaps?

Apologies -- can you go into more details why that would be better?

Long long ago in Rust there were subtle distinctions between how &mut T and &T worked, and I forget if any of them are still true today. Basically because this is a mutable iterator it's "better" to have a phantom data of &mut ... instead of &... to ensure this information is conveyed to the compiler.

Additionally I figure it's best to store FuturesUnordered<F> as that's effectively what's happening here (vs just F). This can also cause subtle differences but probably not as important as &mut vs not

Thanks for explaining that, changed it!

carllerche · 2017-10-30T16:03:29Z

Which release of rust included pub(crate)? I would guess that we should be able to use it soon if not now.

alexcrichton · 2017-10-30T16:21:53Z

Unsure, but the function can also just be moved

srijs · 2017-10-30T20:00:30Z

@carllerche According to this, pub(crate) was stabilised in 1.18.

@alexcrichton the futures_unordered function is exported, according to the docs: https://docs.rs/futures/0.1.16/futures/stream/fn.futures_unordered.html

alexcrichton · 2017-10-31T21:51:05Z

@srijs right yeah but as is this would also export futures::stream::futures_unordered::futures_unordered (that is, a function in the module). Ideally only futures::stream::futures_unordered would be the exported function.

srijs · 2017-11-01T10:01:48Z

@alexcrichton Implemented your feedback.

Now that we have FromIterator for FuturesUnordered, could it be an option to deprecate the free-standing function, or do still see a use-case for it which is not covered by from_iter?

alexcrichton · 2017-11-01T15:29:51Z

src/stream/mod.rs

@@ -103,7 +103,7 @@ if_std! {
    mod wait;
    mod channel;
    mod split;
-    mod futures_unordered;
+    mod futures_unordered_impl;


Oh I was thinking that we'd still want a futures_unordered module which contained the FuturesUnordered type, for example. I think we'll just want to move the definition of the constructor, futures_unordered, from the module to this stream module

No problem, changed!

alexcrichton · 2017-11-01T15:30:22Z

Ah yeah so for now I personally like the free function for symmetry with the rest of the crate, but eventual deprecation is of course possible!

srijs · 2017-11-02T09:23:43Z

@alexcrichton Feedback addressed, let me know if you'd like me to change anything else!

alexcrichton · 2017-11-02T18:10:07Z

Awesome, thanks!

srijs force-pushed the feat/futures-unordered-iter-mut branch 3 times, most recently from d022efb to f21c2a1 Compare October 22, 2017 05:17

carllerche reviewed Oct 23, 2017

View reviewed changes

srijs force-pushed the feat/futures-unordered-iter-mut branch from f21c2a1 to 1f8339f Compare October 24, 2017 08:53

alexcrichton reviewed Oct 30, 2017

View reviewed changes

srijs force-pushed the feat/futures-unordered-iter-mut branch from 1f8339f to cce3c07 Compare November 1, 2017 10:00

srijs force-pushed the feat/futures-unordered-iter-mut branch from cce3c07 to 8c7a70b Compare November 1, 2017 10:05

alexcrichton reviewed Nov 1, 2017

View reviewed changes

srijs force-pushed the feat/futures-unordered-iter-mut branch from 8c7a70b to 16ee1d3 Compare November 2, 2017 09:22

Implement FuturesUnordered::iter_mut

d59b8a3

srijs force-pushed the feat/futures-unordered-iter-mut branch from 16ee1d3 to d59b8a3 Compare November 2, 2017 09:51

alexcrichton merged commit 5a4bfcb into rust-lang:master Nov 2, 2017

srijs mentioned this pull request Nov 3, 2017

Visit all futures in FuturesUnordered #605

Open

srijs deleted the feat/futures-unordered-iter-mut branch November 3, 2017 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FuturesUnordered::iter_mut #618

Implement FuturesUnordered::iter_mut #618

srijs commented Oct 22, 2017 •

edited

Loading

carllerche left a comment

carllerche Oct 23, 2017

carllerche Oct 23, 2017

srijs commented Oct 24, 2017

alexcrichton commented Oct 30, 2017

alexcrichton Oct 30, 2017

srijs Oct 30, 2017 •

edited

Loading

alexcrichton Oct 31, 2017

srijs Nov 1, 2017

carllerche commented Oct 30, 2017

alexcrichton commented Oct 30, 2017

srijs commented Oct 30, 2017 •

edited

Loading

alexcrichton commented Oct 31, 2017

srijs commented Nov 1, 2017 •

edited

Loading

alexcrichton Nov 1, 2017

srijs Nov 2, 2017

alexcrichton commented Nov 1, 2017

srijs commented Nov 2, 2017

alexcrichton commented Nov 2, 2017

Implement FuturesUnordered::iter_mut #618

Implement FuturesUnordered::iter_mut #618

Conversation

srijs commented Oct 22, 2017 • edited Loading

carllerche left a comment

Choose a reason for hiding this comment

carllerche Oct 23, 2017

Choose a reason for hiding this comment

carllerche Oct 23, 2017

Choose a reason for hiding this comment

srijs commented Oct 24, 2017

alexcrichton commented Oct 30, 2017

alexcrichton Oct 30, 2017

Choose a reason for hiding this comment

srijs Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

alexcrichton Oct 31, 2017

Choose a reason for hiding this comment

srijs Nov 1, 2017

Choose a reason for hiding this comment

carllerche commented Oct 30, 2017

alexcrichton commented Oct 30, 2017

srijs commented Oct 30, 2017 • edited Loading

alexcrichton commented Oct 31, 2017

srijs commented Nov 1, 2017 • edited Loading

alexcrichton Nov 1, 2017

Choose a reason for hiding this comment

srijs Nov 2, 2017

Choose a reason for hiding this comment

alexcrichton commented Nov 1, 2017

srijs commented Nov 2, 2017

alexcrichton commented Nov 2, 2017

srijs commented Oct 22, 2017 •

edited

Loading

srijs Oct 30, 2017 •

edited

Loading

srijs commented Oct 30, 2017 •

edited

Loading

srijs commented Nov 1, 2017 •

edited

Loading