Skip to content
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

Merged
merged 9 commits into from
Apr 23, 2022

Conversation

WaffleLapkin
Copy link
Contributor

This is an attempt to resolve #85

@WaffleLapkin WaffleLapkin marked this pull request as draft September 14, 2021 13:26
@WaffleLapkin WaffleLapkin mentioned this pull request Dec 6, 2021
@hawkw
Copy link
Member

hawkw commented Jan 6, 2022

Sorry this took so long to review --- this seems like a reasonable approach! @WaffleLapkin are you interested in continuing to work on this?

@WaffleLapkin
Copy link
Contributor Author

Yes, I'm still interested!

This function allows creating `Arc<T>` where `T: !Sized` by unsizing the
std's `Arc` first.
@WaffleLapkin WaffleLapkin force-pushed the support_arc_with_unsized_valuet branch from 6645f98 to b538d17 Compare January 7, 2022 16:44
(remove them from the global map on drop of the last `Arc`)
@WaffleLapkin WaffleLapkin marked this pull request as ready for review January 7, 2022 16:49
@WaffleLapkin
Copy link
Contributor Author

@hawkw I believe this is ready for review

@WaffleLapkin
Copy link
Contributor Author

(@hawkw friendly ping)

src/sync/arc.rs Show resolved Hide resolved
src/sync/arc.rs Outdated Show resolved Hide resolved
src/sync/arc.rs Outdated Show resolved Hide resolved
src/sync/arc.rs Outdated Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@carllerche
Copy link
Member

@hawkw what is left for this PR?

Copy link
Member

@hawkw hawkw left a 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 Show resolved Hide resolved
src/sync/arc.rs Outdated Show resolved Hide resolved
src/sync/arc.rs Outdated Show resolved Hide resolved
src/sync/arc.rs Outdated
/// use loom::sync::Arc;
///
/// # loom::model::model(|| {
/// // std's arc can be automatically coerced
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe

Suggested change
/// // std's arc can be automatically coerced
/// // std's arc can be automatically coerced from a `!Sized` value

Copy link
Contributor Author

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"?

Copy link
Member

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
Comment on lines 47 to 54
/// # 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);
/// # });
Copy link
Member

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.

@WaffleLapkin
Copy link
Contributor Author

@hawkw I believe I've addressed your nitpicks :)

Copy link
Member

@hawkw hawkw left a 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!

@hawkw hawkw merged commit d0c1e71 into tokio-rs:master Apr 23, 2022
@WaffleLapkin WaffleLapkin deleted the support_arc_with_unsized_valuet branch April 23, 2022 16:04
@WaffleLapkin
Copy link
Contributor Author

Yay! 🎉

hawkw added a commit that referenced this pull request May 10, 2022
# 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)
@hawkw hawkw mentioned this pull request May 10, 2022
hawkw added a commit that referenced this pull request May 13, 2022
# 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arc requires T: Sized
3 participants