Skip to content

Add internal collect_into_array[_unchecked] to remove duplicate code - #82098

Merged
bors merged 1 commit into
rust-lang:masterfrom
LukasKalbertodt:add-collect-into-array
Feb 22, 2021
Merged

Add internal collect_into_array[_unchecked] to remove duplicate code#82098
bors merged 1 commit into
rust-lang:masterfrom
LukasKalbertodt:add-collect-into-array

Conversation

@LukasKalbertodt

@LukasKalbertodtLukasKalbertodt commented Feb 14, 2021

Copy link
Copy Markdown
Contributor

Unlike the similar PRs #69985, #75644 and #79659, this PR only adds private functions and does not propose any new public API. The change is just for the purpose of avoiding duplicate code.

Many array methods already contained the same kind of code and there are still many array related methods to come (e.g. Iterator::{chunks, map_windows, next_n, ...}, [T; N]::{cloned, copied, ...}, ...) which all basically need this functionality. Writing custom unsafe code for each of those doesn't seem like a good idea. I added two functions in this PR (and not just the unsafe version) because I already know that I need the Option-returning version for Iterator::map_windows.

This is closely related to #81615. I think that all options listed in that issue can be implemented using the function added in this PR. The only instance where collect_array_into might not be general enough is when the caller want to handle incomplete arrays manually. Currently, if iter yields fewer than N items, None is returned and the already yielded items are dropped. But as this is just a private function, it can be made more general in future PRs.

And while this was not the goal, this seems to lead to better assembly for array::map: https://rust.godbolt.org/z/75qKTa (CC @JulianKnodt)

Let me know what you think :)

CC @matklad@bstrie

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @dtolnay

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2021
Comment threadlibrary/core/src/array/mod.rs Outdated
@m-ou-sem-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 14, 2021
Comment threadlibrary/core/src/array/mod.rs Outdated
Comment threadlibrary/core/src/array/mod.rs Outdated
Comment threadlibrary/core/src/array/mod.rs Outdated
@scottmcm

Copy link
Copy Markdown
Member

Very nice! I was going to propose my approach from #75571 (comment) (https://godbolt.org/z/e1GdEM), but it looks like the one you have here is giving the same elegance (https://rust.godbolt.org/z/66nrWY):

definevoid@_ZN7example12map_ints_new17hd42cfbe0c1377431E([8 x float]* noaliasnocapturesretdereferenceable(32) %0, [8 x i32]* noaliasnocapturereadonlydereferenceable(32) %s) unnamed_addr #0personalityi32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality!dbg!166 {
start:
%1 = bitcast [8 x i32]* %sto <8 x i32>*, !dbg!167%2 = load <8 x i32>, <8 x i32>* %1, align4, !dbg!167%3 = uitofp <8 x i32> %2to <8 x float>, !dbg!168%4 = bitcast [8 x float]* %0to <8 x float>*, !dbg!180store <8 x float> %3, <8 x float>* %4, align4, !dbg!180, !alias.scope!181, !noalias!184retvoid, !dbg!186
}

Consider whether it'd be worth having a codegen test for one of these examples, since it seems very easy for small permutations to the code to give much worse output...

Comment threadlibrary/core/src/array/mod.rs Outdated
Comment threadlibrary/core/src/array/mod.rs Outdated
@bstrie

Copy link
Copy Markdown
Contributor

I'll second scottmcm's idea of adding a codegen test, if it's not too onerous.

This does not suggest adding such a function to the public API. This is
just for the purpose of avoiding duplicate code. Many array methods
already contained the same kind of code and there are still many array
related methods to come (e.g. `Iterator::{chunks, map_windows, next_n,
...}`) which all basically need this functionality. Writing custom
`unsafe` code for each of those seems not like a good idea.
/// `next` at most `N` times, the iterator can still be used afterwards to
/// retrieve the remaining items.
///
/// If `iter.next()` panicks, all items already yielded by the iterator are

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If `iter.next()` panicks, all items already yielded by the iterator are
/// If `iter.next()` panics, all items already yielded by the iterator are

@dtolnaydtolnay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

@dtolnay

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Feb 20, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit c675af8 has been approved by dtolnay

@borsbors 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 Feb 20, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 20, 2021
…ray, r=dtolnay
Add internal `collect_into_array[_unchecked]` to remove duplicate code
Unlike the similar PRs rust-lang#69985, rust-lang#75644 and rust-lang#79659, this PR only adds private functions and does not propose any new public API. The change is just for the purpose of avoiding duplicate code.
Many array methods already contained the same kind of code and there are still many array related methods to come (e.g. `Iterator::{chunks, map_windows, next_n, ...}`, `[T; N]::{cloned, copied, ...}`, ...) which all basically need this functionality. Writing custom `unsafe` code for each of those doesn't seem like a good idea. I added two functions in this PR (and not just the `unsafe` version) because I already know that I need the `Option`-returning version for `Iterator::map_windows`.
This is closely related to rust-lang#81615. I think that all options listed in that issue can be implemented using the function added in this PR. The only instance where `collect_array_into` might not be general enough is when the caller want to handle incomplete arrays manually. Currently, if `iter` yields fewer than `N` items, `None` is returned and the already yielded items are dropped. But as this is just a private function, it can be made more general in future PRs.
And while this was not the goal, this seems to lead to better assembly for `array::map`: https://rust.godbolt.org/z/75qKTa (CC `@JulianKnodt)`
Let me know what you think :)
CC `@matklad` `@bstrie`
@JohnTitorJohnTitor mentioned this pull request Feb 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2021
Rollup of 9 pull requests
Successful merges:
- rust-lang#82098 (Add internal `collect_into_array[_unchecked]` to remove duplicate code)
- rust-lang#82228 (Provide NonZero_c_* integers)
- rust-lang#82287 (Make "missing field" error message more natural)
- rust-lang#82351 (Use the first paragraph, instead of cookie-cutter text, for rustdoc descriptions)
- rust-lang#82353 (rustdoc: Remove unnecessary `Cell` around `param_env`)
- rust-lang#82367 (remove redundant option/result wrapping of return values)
- rust-lang#82372 (improve UnsafeCell docs)
- rust-lang#82379 (Fix sizes of repr(C) enums on hexagon)
- rust-lang#82382 (rustdoc: Remove `fake_def_ids` RefCell)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 3cf201f into rust-lang:masterFeb 22, 2021
@rustbotrustbot added this to the 1.52.0 milestone Feb 22, 2021
@LukasKalbertodt

Copy link
Copy Markdown
ContributorAuthor

Forgot to check back in here. Now the typo was merged, but I guess that's not a huge disaster for a private function.

I also tried to add codegen checks, as suggested, but I don't have any experience with those and ultimately couldn't get it to work in the time I had. Of course, if someone wants to add those still, that would be helpful. See #75243 for more information about the codegen of array::map.

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

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@LukasKalbertodt@rust-highfive@scottmcm@bstrie@dtolnay@bors@fbstj@the8472@lukaslueg@pickfire@m-ou-se@rustbot