-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
59629ff
to
5606c8e
Compare
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... |
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.
5606c8e
to
c675af8
Compare
/// `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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If `iter.next()` panicks, all items already yielded by the iterator are | |
/// If `iter.next()` panics, all items already yielded by the iterator are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@bors r+ |
📌 Commit c675af8 has been approved by |
…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`
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
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 |
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 customunsafe
code for each of those doesn't seem like a good idea. I added two functions in this PR (and not just theunsafe
version) because I already know that I need theOption
-returning version forIterator::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, ifiter
yields fewer thanN
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