Implement From<&[T]> and others for Arc/Rc (RFC 1845) by murarth · Pull Request #42565 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation60 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

murarth

Tracking issue: #40475

@rust-highfive

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@nikomatsakis

cc @rust-lang/libs -- not sure who is best suited to review this, or what policy you all want to use to decide, but this is splitting up crates and moving things around and I don't think I'm the right person for it. =)

@Mark-Simulacrum

Assigning to @sfackler from libs team to decide on whether we want this.

arielb1

}
impl<T: ?Sized> Arc {
fn from_box(v: Box) -> Arc {

Choose a reason for hiding this comment

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

why isn't this using allocate_slice? I imagine an

unsafe fn for_raw_ptr(ptr: *const T) -> *mut ArcInner { }

That mirrors the unsized part of ptr

Choose a reason for hiding this comment

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

allocate_slice uses the slice length to calculate the size of the allocation. For general unsized T, size is determined differently. That determination is only made in from_box, so it didn't seem to warrant another function.
Also, offset is used to calculate size, but also must be in scope for the call to ptr::copy_nonoverlapping.

I suppose that both T: ?Sized and [T] could use a common allocation function (like for_raw_ptr). Allocating [T] uses a trick (called "dubious" in an existing comment) to get alignment, but the speed advantage over manually calculating it is likely negligible.

Choose a reason for hiding this comment

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

@murarth

The align_of trick is a bit ugly, but I think it's better than hardcoding layout calculations (as allocate_for_ptr does) and is probably going to be well-defined.

I think the "official" way of creating "franken-raw-pointers" is by transmuting to std::raw::TraitObject rather than odd ptr::writes etc.

Also allocate_for_ptr & init_rcbox are always used together. I'll rather merge them and create a ready-to-be-used RcBox.

Choose a reason for hiding this comment

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

I picked up the "franken-raw-pointer" method from the RFC's example implementation. It's used, I think, because the unsized T may be a trait object or a slice or perhaps some theoretical unsized type that is structured differently from either of those. Still, it can be changed to TraitObject if that reads better (which happens to work for slice, too).

I agree that calculating offset is not great, but the offset_of macro fails to compile with unsized T and I didn't see a way around it.

Choose a reason for hiding this comment

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

Ah, it turns out the Franken-pointer method is used because a ?Sized T might, in fact, be sized, which means that transmute to TraitObject will fail. So, the weird ptr::write thing correctly handles a fat pointer or a normal raw pointer.

@sfackler

I'll defer to @alexcrichton on the implementation details - I haven't done enough work with DST representations.

@clarfonthey

So I haven't read 100% of this but I'm a little confused why exactly this is being moved to another crate to depend on libcollections. I didn't have to do anything of the sort to implement From<&[T]> for Box<[T]> and I don't understand why we have to do this for Arc.

At first glance it looks a bit like you're using Vec to do these conversions when there already exists mechanisms for doing it without Vec.

Mind providing a bit more explanation on that @murarth ?

(in general, it makes sense to not make liballoc depend on libcollections unless they're just combined together entirely. although I do think that Arc and Rc should be re-exported in libcollections though)

@clarfonthey

So I looked through the code again and I'm definitely not convinced that this needs to depend on libcollections at all. I think that separating alloc and shared_ptr is a bit too much complexity.

@murarth

@clarcharr: The RFC specifies implementing From<Vec<T>> for Arc/Rc<[T]>. From implementations on &[T], &str, and Box<T>, indeed, do not require a dependency on collections. If the decision is made not to move Arc/Rc to their own separate crate, I can revert the change and drop the From<Vec<T>> implementation. The others do not depend on Vec or collections in any way.

@clarfonthey

@murarth ah, I see. I don't really think that this is a good enough reason to warrant changing crates at the moment; in the future we should be able to put the implementation in libcollections directly.

Right now all of the Vec <-> Box conversions are done using inherent methods and I think that something similar should be done for Rc and Arc.

@bors

☔ The latest upstream changes (presumably #42419) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

Thanks for the PR @murarth! My main point of hesitation here is the addition of a crate rather than the removal of a crate. I feel similarly to others in #40475 that the best solution to this is to just merge the alloc and collections crates. Historically it was believed that alloc and collections would remain separate because collections don't necessarily need a global allocator, but I think the ship has long since sailed here.

Would you be up for deleting the collections crate and moving all of its content to liballoc? I would also be ok doing that outside this PR if you'd prefer to not tackle it just here. That way we can (a) merge the crates and then (b) land this PR. Alternatively we could land all these impls here except From<Vec<T>> for Rc<T> for now, and then add that when the crates are merged.

What do you think?

@murarth

@alexcrichton: Merging collections and alloc does appear to be the most agreeable solution. I think it would be best done in a separate PR and I'll gladly get started on that right away.

@alexcrichton

Ok! So to clarify, @murarth you're thinking we should basically put this PR "on the backburner" until the merge goes through? I'd still be ok laying some groundwork here (e.g. with From<&[T]> for Rc<T>) if you'd like! That way some parallel work could happen instead of it all being serialized.

@murarth

@alexcrichton: Yes, I think it will be less work in the end just to wait for the libcollections merge to land and rebase this on top of it. I'm not in any hurry to get things merged as quickly as possible.

@alexcrichton

Ok sounds good to me! Feel free to r? me on a PR-for-a-merge and I'll try to review quickly as it'd be susceptible to bitrot.

@bors

☔ The latest upstream changes (presumably #42433) made this pull request unmergeable. Please resolve the merge conflicts.

@murarth

I did some tinkering and found a basis for an unsized-capable offset_of to replace manual layout calculation: https://is.gd/zHAJTm

The "elaborate" version gives the same result, but avoids overflowing the pointer if the value happens to be at the very end of the address space. I'm not sure if that's a realistic condition that's worth checking for.

bors added a commit that referenced this pull request

Jun 15, 2017

@bors

Merge crate collections into alloc

This is a necessary step in order to merge #42565

@murarth

Rebased. Also significantly simplified allocate_for_ptr using the above described method.

alexcrichton

Choose a reason for hiding this comment

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

Looking great to me, thanks @murarth!

let v_ptr = &v[..] as *const [T];
let ptr = Self::allocate_for_ptr(v_ptr);
ptr::copy_nonoverlapping(

Choose a reason for hiding this comment

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

Should this use Arc::from_box(v.into_boxed_slice()) perhaps?

Choose a reason for hiding this comment

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

Or alternatively, could this call copy_from_slice (or share the implementation there) before it's followed by set_len(0)?

}
impl<T: Clone> Arc<[T]> {
fn clone_from_slice(v: &[T]) -> Arc<[T]> {

Choose a reason for hiding this comment

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

This and copy_from_slice are only used in one location, right? Could they just be inlined into From below?

#[unstable(feature = "shared_from_slice", issue = "40475")]
impl<'a, T: Clone> From<&'a [T]> for Arc<[T]> {
#[inline]
default fn from(v: &[T]) -> Arc<[T]> {

Choose a reason for hiding this comment

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

We currently try to avoid exposing the usage of specialization right now, mind changing this to use a local ArcFromSlice trait which can be locally specialized?

@@ -1211,8 +1360,49 @@ impl From for Arc {
}
}
#[unstable(feature = "shared_from_slice", issue = "40475")]

Choose a reason for hiding this comment

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

Right now we unfortunately don't have stability checking on impl blocks, so you can go ahead and tag these impls as #[stable]

}
#[unstable(feature = "shared_from_slice", issue = "40475")]
impl<'a> From<&'a str> for Arc {

Choose a reason for hiding this comment

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

While we're at it, should we perhaps have From<String> for Arc<str> as well?

impl<'a> From<&'a str> for Arc {
#[inline]
fn from(v: &str) -> Arc {
unsafe { from_arc_utf8_unchecked(Arc::copy_from_slice(v.as_bytes())) }

Choose a reason for hiding this comment

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

I'd personally be ok just using transmute here, exposing from_arc_utf8_unchecked is unlikely to be stabilized in the long run I think.

@alexcrichton

Looks fantastic! If it's ok with you I'd like to double-check with @rust-lang/libs though that we're ok on merging this. I'm going to tag this as S-waiting-on-team and this'll come up during our typical triage which will occur next Tuesday.

@aturon

@bors

📌 Commit 8e0d01b has been approved by aturon

@bors

⌛ Testing commit 8e0d01b with merge 944675da03018dc4746039b8076dc169fe26ae22...

@bors

@kennytm

@bors

⌛ Testing commit 8e0d01b with merge 6decb846e857cfb9f99f2a3c74834be18d69cf0b...

@bors

@kennytm

@bors retry

A Mac is unscheduled, an another Mac failed to download the LLVM archive correctly.

[00:00:38] Attempting with retry: sh -c rm -f d9e7d2696e41983b6b5a0b4fac604b4e548a84d3.tar.gz &&             curl -sSL -O https://github.com/rust-lang/llvm/archive/d9e7d2696e41983b6b5a0b4fac604b4e548a84d3.tar.gz
[00:00:45] tar: Unrecognized archive format
[00:00:45] tar: Error exit delayed from previous errors.

BTW Travis is having problem with Macs currently:

To address the infrastructure instability and reduced capacity issues on OSX builds, we need to do an emergency maintenance bringing all running OSX builds down. The jobs will be restarted as soon as there is a slot free.

@bors

⌛ Testing commit 8e0d01b with merge 2aabed68e72aa9c0e64f6b5cfd40aea152ee4cb7...

@bors

@alexcrichton

@bors

bors added a commit that referenced this pull request

Aug 23, 2017

@bors

Implement From<&[T]> and others for Arc/Rc (RFC 1845)

Tracking issue: #40475

@bors

@est31 est31 mentioned this pull request

Sep 9, 2017

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Nov 3, 2017

@ryoon

Changelog: Version 1.21.0 (2017-10-12)

Language

Compiler

Libraries

Stabilized APIs

[std::mem::discriminant]

Cargo

Misc

Compatibility Notes

[info/43880]: rust-lang/rust#44224 (comment) [std::mem::discriminant]: https://doc.rust-lang.org/std/mem/fn.discriminant.html

@RalfJung

Turns out this has a soundness bug :( #54908

plotskogwq added a commit to plotskogwq/curve25519-dalek that referenced this pull request

Aug 10, 2024

@plotskogwq @tonychain

libcollections was recently merged into liballoc:

rust-lang/rust#42648

I went ahead and also added an "alloc" feature which no_std users can use to opt into liballoc features (i.e. any code using Vec). This should have no effect on anything but no_std usage. It does make it possible for people without allocators to use curve25519-dalek if they want though. Might be nice for "bare metal" development.

All that said, from what I can gather liballoc, while not "stable", should likely stick around for the forseeable future.

Some backstory on the liballoc/libcollections merge here:

rust-lang/rust#42565

Labels

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.