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

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

Merged
merged 1 commit into from Aug 24, 2017

Conversation

Projects
None yet
@murarth
Contributor

murarth commented Jun 9, 2017

  • 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

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jun 9, 2017

Collaborator

r? @nikomatsakis

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

Collaborator

rust-highfive commented Jun 9, 2017

r? @nikomatsakis

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

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 9, 2017

Contributor

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. =)

Contributor

nikomatsakis commented Jun 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jun 11, 2017

Member

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

Member

Mark-Simulacrum commented Jun 11, 2017

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

Show outdated Hide outdated src/libshared_ptr/arc.rs Outdated
@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 11, 2017

Member

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

Member

sfackler commented Jun 11, 2017

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

@sfackler sfackler assigned alexcrichton and unassigned sfackler Jun 11, 2017

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jun 12, 2017

Contributor

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)

Contributor

clarcharr commented Jun 12, 2017

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)

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jun 12, 2017

Contributor

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.

Contributor

clarcharr commented Jun 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@murarth

murarth Jun 12, 2017

Contributor

@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.

Contributor

murarth commented Jun 12, 2017

@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.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jun 12, 2017

Contributor

@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.

Contributor

clarcharr commented Jun 12, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 12, 2017

Contributor

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

Contributor

bors commented Jun 12, 2017

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 13, 2017

Member

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?

Member

alexcrichton commented Jun 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@murarth

murarth Jun 13, 2017

Contributor

@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.

Contributor

murarth commented Jun 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 13, 2017

Member

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.

Member

alexcrichton commented Jun 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@murarth

murarth Jun 13, 2017

Contributor

@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.

Contributor

murarth commented Jun 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 13, 2017

Member

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.

Member

alexcrichton commented Jun 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 14, 2017

Contributor

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

Contributor

bors commented Jun 14, 2017

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

@murarth

This comment has been minimized.

Show comment
Hide comment
@murarth

murarth Jun 14, 2017

Contributor

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.

Contributor

murarth commented Jun 14, 2017

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

Auto merge of #42648 - murarth:merge-alloc-collections, r=alexcrichton
Merge crate `collections` into `alloc`

This is a necessary step in order to merge #42565
@murarth

This comment has been minimized.

Show comment
Hide comment
@murarth

murarth Jun 15, 2017

Contributor

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

Contributor

murarth commented Jun 15, 2017

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

@alexcrichton

Looking great to me, thanks @murarth!

Show outdated Hide outdated src/liballoc/arc.rs Outdated
Show outdated Hide outdated src/liballoc/arc.rs Outdated
Show outdated Hide outdated src/liballoc/arc.rs Outdated
Show outdated Hide outdated src/liballoc/arc.rs Outdated
Show outdated Hide outdated src/liballoc/arc.rs Outdated
Show outdated Hide outdated src/liballoc/arc.rs Outdated
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 16, 2017

Member

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.

Member

alexcrichton commented Jun 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aturon
Member

aturon commented Aug 22, 2017

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 22, 2017

Contributor

📌 Commit 8e0d01b has been approved by aturon

Contributor

bors commented Aug 22, 2017

📌 Commit 8e0d01b has been approved by aturon

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

⌛️ Testing commit 8e0d01b with merge 944675d...

Contributor

bors commented Aug 23, 2017

⌛️ Testing commit 8e0d01b with merge 944675d...

bors added a commit that referenced this pull request Aug 23, 2017

Auto merge of #42565 - murarth:rc-from-slice, r=aturon
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
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Aug 23, 2017

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Aug 23, 2017

@bors retry #38618

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

⌛️ Testing commit 8e0d01b with merge 6decb84...

Contributor

bors commented Aug 23, 2017

⌛️ Testing commit 8e0d01b with merge 6decb84...

bors added a commit that referenced this pull request Aug 23, 2017

Auto merge of #42565 - murarth:rc-from-slice, r=aturon
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
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Aug 23, 2017

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Aug 23, 2017

Member

@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.

Member

kennytm commented Aug 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

⌛️ Testing commit 8e0d01b with merge 2aabed6...

Contributor

bors commented Aug 23, 2017

⌛️ Testing commit 8e0d01b with merge 2aabed6...

bors added a commit that referenced this pull request Aug 23, 2017

Auto merge of #42565 - murarth:rc-from-slice, r=aturon
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
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Aug 23, 2017

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 23, 2017

Member

@bors: retry

  • spurious failure of a test that failed to start #43283
Member

alexcrichton commented Aug 23, 2017

@bors: retry

  • spurious failure of a test that failed to start #43283
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2017

Contributor

⌛️ Testing commit 8e0d01b with merge 560b6ca...

Contributor

bors commented Aug 23, 2017

⌛️ Testing commit 8e0d01b with merge 560b6ca...

bors added a commit that referenced this pull request Aug 23, 2017

Auto merge of #42565 - murarth:rc-from-slice, r=aturon
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
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 24, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 560b6ca to master...

Contributor

bors commented Aug 24, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 560b6ca to master...

@bors bors merged commit 8e0d01b into rust-lang:master Aug 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@murarth murarth deleted the murarth:rc-from-slice branch Aug 24, 2017

@est31 est31 referenced this pull request Sep 9, 2017

Closed

im beta regression #44446

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 3, 2017

Update to 1.21.0
Changelog:
Version 1.21.0 (2017-10-12)
==========================

Language
--------
- [You can now use static references for literals.][43838]
  Example:
  ```rust
  fn main() {
      let x: &'static u32 = &0;
  }
  ```
- [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540]
  Example:
  ```rust
  my_macro!(Vec<i32>::new); // Always worked
  my_macro!(Vec::<i32>::new); // Now works
  ```

Compiler
--------
- [Upgraded jemalloc to 4.5.0][43911]
- [Enabled unwinding panics on Redox][43917]
- [Now runs LLVM in parallel during translation phase.][43506]
  This should reduce peak memory usage.

Libraries
---------
- [Generate builtin impls for `Clone` for all arrays and tuples that
  are `T: Clone`][43690]
- [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459]
- [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`,
  `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565]

Stabilized APIs
---------------

[`std::mem::discriminant`]

Cargo
-----
- [You can now call `cargo install` with multiple package names][cargo/4216]
- [Cargo commands inside a virtual workspace will now implicitly
  pass `--all`][cargo/4335]
- [Added a `[patch]` section to `Cargo.toml` to handle
  prepublication dependencies][cargo/4123] [RFC 1969]
- [`include` & `exclude` fields in `Cargo.toml` now accept gitignore
  like patterns][cargo/4270]
- [Added the `--all-targets` option][cargo/4400]
- [Using required dependencies as a feature is now deprecated and emits
  a warning][cargo/4364]


Misc
----
- [Cargo docs are moving][43916]
  to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo)
- [The rustdoc book is now available][43863]
  at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc)
- [Added a preview of RLS has been made available through rustup][44204]
  Install with `rustup component add rls-preview`
- [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348]
  Previously only showed `std::os::unix`.

Compatibility Notes
-------------------
- [Changes in method matching against higher-ranked types][43880] This may cause
  breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880]
- [rustc's JSON error output's byte position start at top of file.][42973]
  Was previously relative to the rustc's internal `CodeMap` struct which
  required the unstable library `libsyntax` to correctly use.
- [`unused_results` lint no longer ignores booleans][43728]

[42565]: rust-lang/rust#42565
[42973]: rust-lang/rust#42973
[43348]: rust-lang/rust#43348
[43459]: rust-lang/rust#43459
[43506]: rust-lang/rust#43506
[43540]: rust-lang/rust#43540
[43690]: rust-lang/rust#43690
[43728]: rust-lang/rust#43728
[43838]: rust-lang/rust#43838
[43863]: rust-lang/rust#43863
[43880]: rust-lang/rust#43880
[43911]: rust-lang/rust#43911
[43916]: rust-lang/rust#43916
[43917]: rust-lang/rust#43917
[44204]: rust-lang/rust#44204
[cargo/4123]: rust-lang/cargo#4123
[cargo/4216]: rust-lang/cargo#4216
[cargo/4270]: rust-lang/cargo#4270
[cargo/4335]: rust-lang/cargo#4335
[cargo/4364]: rust-lang/cargo#4364
[cargo/4400]: rust-lang/cargo#4400
[RFC 1969]: rust-lang/rfcs#1969
[info/43880]: rust-lang/rust#44224 (comment)
[`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 8, 2018

Member

Turns out this has a soundness bug :( #54908

Member

RalfJung commented Oct 8, 2018

Turns out this has a soundness bug :( #54908

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment