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.

Sign up for GitHub

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

Jump to bottom

Replace iterator-based construction of collections by Into<T> #91861

Merged
merged 1 commit into from
Jan 27, 2022
Merged

Replace iterator-based construction of collections by Into<T> #91861

merged 1 commit into from
Jan 27, 2022

Conversation

juniorbassani
Copy link
Contributor

Just a few quality of life improvements in the doc examples. I also removed some Vecs in favor of arrays.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@juniorbassani juniorbassani force-pushed the use-from-array-in-collections-examples branch from 3ba1ff7 to d2642cb Compare December 13, 2021 14:06
@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 13, 2021
nrc
nrc reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! These look like great improvements (especially the BTreeMap one, I was pleasantly surprised that works!), however (as I commented inline) I think it would be better to use into rather than from where possible.

library/alloc/src/collections/vec_deque/mod.rs Outdated
@@ -2547,7 +2547,7 @@ impl<T, A: Allocator> VecDeque<T, A> {
/// ```
/// use std::collections::VecDeque;
///
/// let deque: VecDeque<_> = vec![0, 1, 1, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55].into();
/// let deque = VecDeque::from([0, 1, 1, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55]);
Copy link
Member

Choose a reason for hiding this comment

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

The docs recommend using into over from, so could we keep using into in these cases, but drop the vec!? (And replace the various iterator constructs with into rather than from too)

@juniorbassani
Copy link
Contributor Author

Thanks for the PR! These look like great improvements (especially the BTreeMap one, I was pleasantly surprised that works!), however (as I commented inline) I think it would be better to use into rather than from where possible.

I wouldn't mind replacing from with into, but all the other collections not covered in this PR are already using from in the doc examples. In this case, should we change all of them to use into? Otherwise, the documentation would seem inconsistent.

@nrc
Copy link
Member

nrc commented Dec 20, 2021

I wouldn't mind replacing from with into, but all the other collections not covered in this PR are already using from in the doc examples. In this case, should we change all of them to use into? Otherwise, the documentation would seem inconsistent.

That's a good question! Since it is a big question and somewhat orthogonal to the goal of this PR, I would work around it here and discuss separately (i.e., open an issue or PR if you are interested). For this PR, I would leave the from/into part of each example and just remove the iterator-ish boilerplate.

@juniorbassani
Copy link
Contributor Author

juniorbassani commented Jan 11, 2022 •
edited
Loading

Thanks for the suggestion, @nrc. I'm just uncertain about the intermediate vec allocations. Should I leave them unchanged or can I replace them with stack-based arrays?

Edit: I suppose they should be replaced, as #92070 already did that in multiple files.

@bors
Copy link
Contributor

bors commented Jan 11, 2022

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

@juniorbassani juniorbassani force-pushed the use-from-array-in-collections-examples branch from d2642cb to 0577401 Compare January 18, 2022 14:14
@juniorbassani juniorbassani changed the title Replace iterator-based construction of collections by From<[T; N]> Replace iterator-based construction of collections by Into<T> Jan 18, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@juniorbassani juniorbassani force-pushed the use-from-array-in-collections-examples branch from 0577401 to 8936659 Compare January 18, 2022 15:18
@juniorbassani
Copy link
Contributor Author

@nrc, I've updated the PR. Now, the intermediate Vec allocations were removed, and the iterators were replaced with Into.

Let me know if there's anything else that could be improved.

@nrc
Copy link
Member

nrc commented Jan 26, 2022

lgtm, thanks for the changes @juniorbassani !

@yaahc could you give an official review please?

@yaahc
Copy link
Member

yaahc commented Jan 26, 2022

Looks great to me, thank you very much, both of you!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2022

📌 Commit 8936659 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 26, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2022
@bors
Auto merge of rust-lang#93352 - matthiaskrgr:rollup-5peret4, r=matthi…
009c1d0 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90247 (Improve Duration::try_from_secs_f32/64 accuracy by directly processing exponent and mantissa)
 - rust-lang#91861 (Replace iterator-based construction of collections by `Into<T>`)
 - rust-lang#92098 (add OpenBSD platform-support page)
 - rust-lang#92134 (Add x86_64-pc-windows-msvc linker-plugin-lto instructions)
 - rust-lang#92256 (Improve selection errors for `~const` trait bounds)
 - rust-lang#92778 (fs: Use readdir() instead of readdir_r() on Linux and Android)
 - rust-lang#93338 (Update minifier crate version to 0.0.42)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1dd0ac1 into rust-lang:master Jan 27, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 27, 2022
clint-white added a commit to clint-white/binary-heap-plus-rs that referenced this pull request Aug 7, 2022
@clint-white
Port updated example from rust-lang/rust
ae09fe7 
This commits ports one small part of rust-lang/rust#91861.  The main
change that PR made to `binary_heap.rs` was to replace vectors with
arrays in the examples.  We do *not* port such changes as they require
Rust 1.56.0, which is greater than this crate's current MSRV.  However,
it also simplified the setup in the example of `BinaryHeap::append()`,
and we repeat that here only with vectors instead of arrays.
clint-white added a commit to clint-white/binary-heap-plus-rs that referenced this pull request Sep 23, 2022
@clint-white
Use arrays instead of Vecs in documentation examples
250444e 
This change is ported from rust-lang/rust#91861.  Also finish porting
rust-lang/rust#84111, partially ported here in sekineh#34, by including the
example of constructing a binary heap from an array.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc left review comments

Assignees

@yaahc yaahc

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@juniorbassani @rust-highfive @rust-log-analyzer @nrc @bors @yaahc @camelid @rustbot