Fix Merge System #243
Fix Merge System #243
Conversation
src/common.rs
Outdated
| } | ||
| } | ||
|
|
||
| static NOTIFY_IGNORE: &&'static NotifyIgnore = &&NotifyIgnore; |
There was a problem hiding this comment.
No need for lifetimes in static items.
src/common.rs
Outdated
| self.spawns.push(RefCell::new((e, spawn(futures.remove(e).unwrap())))); | ||
| } | ||
|
|
||
| self.spawns.retain(|spawn| { |
There was a problem hiding this comment.
In order to get rid of RefCell, we can use
let len = spawns.len();
let mut del = 0;
{
for i in 0..len {
let remove = { /* Do stuff */ };
if remove {
del += 1;
} else if del > 0 {
spawns.swap(i - del, i);
}
}
}
if del > 0 {
spawns.truncate(len - del);
}You can also just make this a helper function. The fact that retain only allows immutable access was a design mistake.
There was a problem hiding this comment.
Ha! That's basically exactly what I wrote for SmallVec's retain function. I love code re-use.
There was a problem hiding this comment.
That is almost like my first variant 😄
src/common.rs
Outdated
| use std::convert::AsRef; | ||
| use std::error::Error; | ||
| use std::fmt::{Debug, Display, Formatter, Result as FmtResult}; | ||
| use std::io::Write; | ||
| use std::marker::PhantomData; | ||
|
|
||
| use futures::{Async, Future}; | ||
| use futures::executor::{Notify, spawn, Spawn}; |
There was a problem hiding this comment.
Please reorder so that spawn is at the end.
src/common.rs
Outdated
| struct NotifyIgnore; | ||
| impl Notify for NotifyIgnore { | ||
| fn notify(&self, _: usize) { | ||
| /* Intentionally ignore */ |
|
When I tried the system out it worked, so yeah, maybe my |
|
It would be really nice to have a test / an example using futures, so we can validate it actually works now. |
|
Shared is a really easy way to generate a panic if this isn't working, as it depends on |
|
Okay, I'm going to merge this, but I would like to add an example before releasing a new version. We could also port this fix back to 0.9 if you want to. bors r+ |
243: Fix Merge System r=torkleyy a=omni-viral Fix `Merge` `System` `Future`s can't be `poll`ed without `Task`. `Spawn` creates such `Task` and provide API to run inner `Future` Without this `Merge` will `panic!` when it'll try to call `Future::poll` except if it is trivial custom `Future`
Build succeeded |
This ports a bug fix by @omni-viral back to 0.9 This includes the following commits: Poll on Spawn, not on Future Do not require Spawn to be Sync Fix typo Get rid of RefCell
Fix
MergeSystemFutures can't bepolled withoutTask.Spawncreates suchTaskand provide API to run innerFutureWithout this
Mergewillpanic!when it'll try to callFuture::pollexcept if it is trivial customFuture