Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upFix a few lifetime issues #36
Conversation
stjepang
reviewed
Jul 26, 2018
| @@ -183,7 +183,7 @@ struct JoinState<T> { | |||
| _marker: PhantomData<T>, | |||
| } | |||
|
|
|||
| impl<T: Send> JoinState<T> { | |||
| impl<T> JoinState<T> { | |||
This comment has been minimized.
This comment has been minimized.
stjepang
Jul 26, 2018
Member
We don't really need this bound because JoinState cannot be created unless T: Send. Note that JoinHandle doesn't have this impl bound either.
stjepang
reviewed
Jul 26, 2018
| @@ -226,7 +229,7 @@ pub struct ScopedJoinHandle<'a, T: 'a> { | |||
| /// ``` | |||
| /// crossbeam_utils::thread::scope(|scope| { | |||
| /// scope.defer(|| println!("Exiting scope")); | |||
| /// scope.spawn(|| println!("Running child thread in scope")) | |||
| /// scope.spawn(|| println!("Running child thread in scope")); | |||
This comment has been minimized.
This comment has been minimized.
stjepang
Jul 26, 2018
Member
Without the semicolon the ScopedJoinHandle was actually returned by the invocation of scope(), thus leaking out of the scope!
stjepang
reviewed
Jul 26, 2018
| @@ -299,7 +302,7 @@ impl<'a> Scope<'a> { | |||
| /// scope exits. | |||
| /// | |||
| /// [spawn]: http://doc.rust-lang.org/std/thread/fn.spawn.html | |||
| pub fn spawn<F, T>(&self, f: F) -> ScopedJoinHandle<'a, T> | |||
| pub fn spawn<'s, F, T>(&'s self, f: F) -> ScopedJoinHandle<'s, T> | |||
This comment has been minimized.
This comment has been minimized.
stjepang
Jul 26, 2018
Member
ScopedJoinHandle must borrow the Scope (lifetime 's), not the environment outside the scope (lifetime 'a).
Previously the following was compiling just fine:
let m = Mutex::new(None);
scope(|s| {
let h = s.spawn(|| ());
*m.lock().unwrap() = Some(h);
});
stjepang
reviewed
Jul 26, 2018
| @@ -340,7 +343,7 @@ impl<'s, 'a: 's> ScopedThreadBuilder<'s, 'a> { | |||
| } | |||
|
|
|||
| /// Spawns a new thread, and returns a join handle for it. | |||
| pub fn spawn<F, T>(self, f: F) -> io::Result<ScopedJoinHandle<'a, T>> | |||
| pub fn spawn<F, T>(self, f: F) -> io::Result<ScopedJoinHandle<'s, T>> | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
One more thing: we should really choose better names for lifetimes Thoughts? |
stjepang
reviewed
Jul 26, 2018
| @@ -208,6 +208,9 @@ pub struct ScopedJoinHandle<'a, T: 'a> { | |||
| _marker: PhantomData<&'a T>, | |||
| } | |||
|
|
|||
| unsafe impl<'a, T> Send for ScopedJoinHandle<'a, T> {} | |||
| unsafe impl<'a, T> Sync for ScopedJoinHandle<'a, T> {} | |||
This comment has been minimized.
This comment has been minimized.
stjepang
Jul 26, 2018
Member
I've also submitted a PR that adds similar impls for JoinHandle:
rust-lang/rust#52759
stjepang commentedJul 26, 2018
More safety problems getting fixed... :)
Explanation below in comments.