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

Tracking issue for From<&[T]> for Rc & co #40475

Closed
alexcrichton opened this issue Mar 13, 2017 · 16 comments
Closed

Tracking issue for From<&[T]> for Rc & co #40475

alexcrichton opened this issue Mar 13, 2017 · 16 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Tracking issue for rust-lang/rfcs#1845

@alexcrichton alexcrichton added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 13, 2017
@alexbool
Copy link
Contributor

Anyone willing to implement this?
@Centril ? :)

@Centril
Copy link
Contributor

Centril commented Mar 29, 2017

@alexbool Sure, I just need to get started with and learn how to contribute to the compiler / stdlib. I'll ask for some help on #rust-internals.

@murarth
Copy link
Contributor

murarth commented Jun 8, 2017

I see it's been quiet here for a couple of months, so I thought I would try implementing this myself and I ran into an issue in the process:

Arc/Rc live in alloc and Vec lives in collections, which depends on alloc. Attempting to implement From<Vec<T>> for Arc<[T]>/Rc<[T]> as the RFC states requires introducing a circular dependency on collections for alloc.

I'm unsure how to proceed. A few options are immediately apparent to me:

  • Move Arc/Rc into a separate crate, which may then have collections as a dependency.
  • Instead implement Into<Arc<[T]>> (and Rc<[T]>) for Vec<T>.
  • Forego implementing From<Vec<T>> altogether. It would still be possible to perform the conversion indirectly, via Vec<T> -> Box<[T]> -> Arc/Rc<[T]>.

@Centril
Copy link
Contributor

Centril commented Jun 8, 2017

@murarth Apologies for my tardiness.
A fourth option is, and which I prefer:

  • Move Vec into alloc

This should be backwards compatible since collections can simply import Vec from alloc.

@SimonSapin
Copy link
Contributor

Move Vec into alloc

In that case what’s the point of having separating alloc and collections crates?

@Centril
Copy link
Contributor

Centril commented Jun 8, 2017

@SimonSapin It can be argued that Vec is more fundamental than the rest of collections, but there is RawVec that already does the "fundamental" part.

Regarding the other choices,

  • Moving Arc / Rc into a separate crate breaks backwards compatibility - but since alloc is experimental, I guess that's fine.
  • Into<Arc> ... is reasonable if no other choice is viable.
  • Forego... don't do this... Into<Arc> is a better choice.

Actually... I change my mind... Moving Arc / Rc into a crate named shared would be a great idea.

@SimonSapin
Copy link
Contributor

Or, move all of alloc and collections into a single crate (to be named like either of them or something else…)

@Centril
Copy link
Contributor

Centril commented Jun 8, 2017

@SimonSapin Hmm... Wouldn't some developers for the embedded domain want the remaining stuff in alloc but perhaps not collections? Are there no downsides to merging them into one crate?

@japaric ^

@murarth
Copy link
Contributor

murarth commented Jun 8, 2017

I'm leaning toward the first option (spinning Arc and Rc out into a separate crate) because it's the only way to fully implement the RFC. I don't think this is something that would typically be called a "breaking change" as the alloc crate is unstable.

@japaric
Copy link
Member

japaric commented Jun 10, 2017

@Centril I'm probably not the best person to ask about this because although I do embedded Rust development I try to avoid using dynamic memory allocation as much as possible.

I don't think this is something that would typically be called a "breaking change" as the alloc crate is unstable.

This is the other reason why I avoid any no_std crate that's not core.

As for the actual question: I wouldn't mind if collections was merged into alloc and see no downside in merging them other than increased compilation time if you only wanted the stuff that's currently in the alloc crate. (Personally I'm more of a fan of having more crates rather than less.)

@clarfonthey
Copy link
Contributor

One thing that could potentially be useful in the future is adding implementations to create Rcs of CStr, OsStr, and Path, like I did for Box (see #40380). I can in particular see Rc<Path> being useful, although in general it'd be nice to have feature parity between Box and Rc and Arc.

@strega-nil
Copy link
Contributor

Can we just implement the &[T] -> Rc<[T]> conversion for now? It's pretty useful, and the functionality is just missing right now...

@SimonSapin
Copy link
Contributor

It’s already implemented. It needs to be exposed in a public API. https://github.com/rust-lang/rust/blob/1.18.0/src/liballoc/rc.rs#L402-L457

@SimonSapin
Copy link
Contributor

Although it’s from Box<[T]>. For &[T] we’d also need a T: Clone bound. Or should it be generic over Iterator<Item=T> + ExactSizeIterator?

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 27, 2017
bors added a commit that referenced this issue Aug 23, 2017
Implement From<&[T]> and others for Arc/Rc (RFC 1845)

* Implements `From<`{`&[T]`, `&str`, `String`, `Box<T> where T: ?Sized`, `Vec<T>`}`>` for `Arc`/`Rc`
* Removes `rustc_private`-marked methods `Rc::__from_array` and `Rc::__from_str`, replacing their use with `Rc::from`

Tracking issue: #40475
@murarth
Copy link
Contributor

murarth commented Aug 24, 2017

Huzzah, the implementation PR #42565 has been merged!

The From impls are insta-stable, so should this be closed or wait until they reach the next stable release?

@alexcrichton
Copy link
Member Author

Indeed this can be stable now, thanks again for sticking with this @murarth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants