-
Notifications
You must be signed in to change notification settings - Fork 111
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
Remove (implicit) T: Sized
bound from Arc
#226
Remove (implicit) T: Sized
bound from Arc
#226
Conversation
d65be29
to
6645f98
Compare
Sorry this took so long to review --- this seems like a reasonable approach! @WaffleLapkin are you interested in continuing to work on this? |
Yes, I'm still interested! |
This function allows creating `Arc<T>` where `T: !Sized` by unsizing the std's `Arc` first.
6645f98
to
b538d17
Compare
(remove them from the global map on drop of the last `Arc`)
@hawkw I believe this is ready for review |
(@hawkw friendly ping) |
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw what is left for this PR? |
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.
overall, this seems great to me --- i left some very minor nitpicky comments, but I think this looks good!
src/sync/arc.rs
Outdated
/// use loom::sync::Arc; | ||
/// | ||
/// # loom::model::model(|| { | ||
/// // std's arc can be automatically coerced |
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.
nit: maybe
/// // std's arc can be automatically coerced | |
/// // std's arc can be automatically coerced from a `!Sized` value |
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.
That's not really correct though, the coercion goes Arc<[u8; 3]>
-> Arc<[u8]>
, so it is not "from a !Sized
value" but more like "to a !Sized
value"?
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.
ah, okay, "to" would also be fine, i just thought it was nice if the comment explicitly stated the coercion that was happening. not a big deal!
src/sync/arc.rs
Outdated
/// # loom::model::model(|| { | ||
/// // std's arc can be automatically coerced | ||
/// let std = std::sync::Arc::new([1, 2, 3]); | ||
/// let loom: Arc<[u8]> = Arc::from_std(std); | ||
/// | ||
/// let std = std::sync::Arc::new([1, 2, 3]); | ||
/// let loom: Arc<dyn Sync + Send> = Arc::from_std(std); | ||
/// # }); |
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.
a very minor docs suggestion, take it or leave it: it could also be nice to have a compile_fail
example showing how loom::sync::Arc::new
cannot be used with !Sized
values directly, so that users can see why this function is necessary? not a blocker.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw I believe I've addressed your nitpicks :) |
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.
docs changes look great, thanks!
Yay! 🎉 |
# 0.5.5 (May 10, 2022) ### Added - sync: Add `Arc::from_std` without `T: Sized` bound (#226) - sync: Implement `Debug` for `AtomicPtr` for all `T` (#255) - logs: Add location tracking for threads and atomic operations (#258) - logs: Add additional location tracking to `Arc`, `alloc`, and `mpsc` (#265) - logs: Improve `tracing` configuration for `LOOM_LOG` (#266) - logs: Add a span for the current model's iteration (#267) ### Documented - Add note about in-memory representation of atomic types (#253) - Document `LOOM_LOG` syntax (#257) ### Fixed - Fix double panic when exceeding the branch limit in `Drop` (#245) - cell: Allow using `{Mut,Const}Ptr::{deref,with}` when the pointee is `!Sized` (#247) - thread: Fix semantics of `thread::park` after `Thread::unpark` (#250)
# 0.5.5 (May 10, 2022) ### Added - sync: Add `Arc::from_std` without `T: Sized` bound (#226) - sync: Implement `Debug` for `AtomicPtr` for all `T` (#255) - logs: Add location tracking for threads and atomic operations (#258) - logs: Add additional location tracking to `Arc`, `alloc`, and `mpsc` (#265) - logs: Improve `tracing` configuration for `LOOM_LOG` (#266) - logs: Add a span for the current model's iteration (#267) ### Documented - Add note about in-memory representation of atomic types (#253) - Document `LOOM_LOG` syntax (#257) ### Fixed - Fix double panic when exceeding the branch limit in `Drop` (#245) - cell: Allow using `{Mut,Const}Ptr::{deref,with}` when the pointee is `!Sized` (#247) - thread: Fix semantics of `thread::park` after `Thread::unpark` (#250)
This is an attempt to resolve #85