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 internal collect_into_array[_unchecked] to remove duplicate code #82098

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Feb 14, 2021

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
Collaborator

r? @dtolnay

(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 Feb 14, 2021
@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 14, 2021
@scottmcm
Copy link
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):

define void @_ZN7example12map_ints_new17hd42cfbe0c1377431E([8 x float]* noalias nocapture sret dereferenceable(32) %0, [8 x i32]* noalias nocapture readonly dereferenceable(32) %s) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality !dbg !166 {
start:
  %1 = bitcast [8 x i32]* %s to <8 x i32>*, !dbg !167
  %2 = load <8 x i32>, <8 x i32>* %1, align 4, !dbg !167
  %3 = uitofp <8 x i32> %2 to <8 x float>, !dbg !168
  %4 = bitcast [8 x float]* %0 to <8 x float>*, !dbg !180
  store <8 x float> %3, <8 x float>* %4, align 4, !dbg !180, !alias.scope !181, !noalias !184
  ret void, !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...

@bstrie
Copy link
Contributor

bstrie commented Feb 15, 2021

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

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Nice!

@dtolnay
Copy link
Member

dtolnay commented Feb 20, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2021

📌 Commit c675af8 has been approved by dtolnay

@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 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`
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:master Feb 22, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 22, 2021
@LukasKalbertodt
Copy link
Member Author

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant 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.