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

Add `impl<T> FromIterator<T> for Arc/Rc<[T]>` #61953

Merged
merged 13 commits into from Jul 13, 2019

Conversation

@Centril
Copy link
Member

commented Jun 19, 2019

Add implementations of FromIterator<T> for Arc/Rc<[T]> with symmetrical logic.

This also takes advantage of specialization in the case of iterators with known length (TrustedLen) to elide the final allocation/copying from a Vec<T> into Rc<[T]> because we can allocate the space for the Rc<[T]> directly when the size is known. This is the primary motivation and why this is to be preferred over iter.collect::<Vec<_>>().into(): Rc<[T]>.

Moreover, this PR does some refactoring in some places.

r? @RalfJung for the code
cc @alexcrichton from T-libs

@Centril

This comment was marked as resolved.

Copy link
Member Author

commented Jun 19, 2019

(#60667 will likely land much sooner; I'll rebase after that has happened)

Done.

@Centril Centril force-pushed the Centril:shared-from-iter branch from 096f5e2 to df5d3a4 Jun 19, 2019

@Centril Centril force-pushed the Centril:shared-from-iter branch from df5d3a4 to 4b44ad9 Jun 20, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Are there tests covering this new code? If not, could you add some? I'd like to run Miri on this.

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@RalfJung There are some doc-tests, but they might be sparse? Does miri run doc-tests?

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

No it does not, unfortunately (rust-lang/miri#584). Could you add #[test] methods?

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Sure; I'll do that and try to exercise some more pathological cases also.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Well done overall! This all looks great, and I wish more code was commented like this.
Just found one comment nit.

I'd like to run Miri though before r+'ing.

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Added some more tests... :)

Show resolved Hide resolved src/liballoc/sync.rs Outdated
@rfcbot

This comment has been minimized.

Copy link

commented Jul 2, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019

@rfcbot

This comment has been minimized.

Copy link

commented Jul 12, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@bors: r=RalfJung

r? @RalfJung

Moving back to the technical reviewer

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

📌 Commit 85def30 has been approved by RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

⌛️ Testing commit 85def30 with merge 7de19ce...

bors added a commit that referenced this pull request Jul 12, 2019

Auto merge of #61953 - Centril:shared-from-iter, r=RalfJung
Add `impl<T> FromIterator<T> for Arc/Rc<[T]>`

Add implementations of `FromIterator<T> for Arc/Rc<[T]>` with symmetrical logic.

This also takes advantage of specialization in the case of iterators with known length (`TrustedLen`) to elide the final allocation/copying from a `Vec<T>` into `Rc<[T]>` because we can allocate the space for the `Rc<[T]>` directly when the size is known. This is the primary motivation and why this is to be preferred over `iter.collect::<Vec<_>>().into(): Rc<[T]>`.

Moreover, this PR does some refactoring in some places.

r? @RalfJung for the code
cc @alexcrichton from T-libs
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

💔 Test failed - checks-azure

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

⌛️ Testing commit 85def30 with merge 4a95e97...

bors added a commit that referenced this pull request Jul 13, 2019

Auto merge of #61953 - Centril:shared-from-iter, r=RalfJung
Add `impl<T> FromIterator<T> for Arc/Rc<[T]>`

Add implementations of `FromIterator<T> for Arc/Rc<[T]>` with symmetrical logic.

This also takes advantage of specialization in the case of iterators with known length (`TrustedLen`) to elide the final allocation/copying from a `Vec<T>` into `Rc<[T]>` because we can allocate the space for the `Rc<[T]>` directly when the size is known. This is the primary motivation and why this is to be preferred over `iter.collect::<Vec<_>>().into(): Rc<[T]>`.

Moreover, this PR does some refactoring in some places.

r? @RalfJung for the code
cc @alexcrichton from T-libs
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: RalfJung
Pushing 4a95e97 to master...

@bors bors added the merged-by-bors label Jul 13, 2019

@bors bors merged commit 85def30 into rust-lang:master Jul 13, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
pr Build #20190621.65 succeeded
Details

@Centril Centril deleted the Centril:shared-from-iter branch Jul 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.