Github Add `[T; N]::each_ref` and `[T; N]::each_mut` by LukasKalbertodt · Pull R...

Member

impl<T, const N: usize> [T; N] {
    pub fn each_ref(&self) -> [&T; N];
    pub fn each_mut(&mut self) -> [&mut T; N];
}

This comment was marked as off-topic.

LukasKalbertodt commented on Aug 14, 2020

Oh no, not this again
[1, 2, 3].as_ref() already works today and returns &[i32]. So adding these methods is a breaking change, very similarly to #65819. Luckily, in this case we can simply change the name. Maybe elements_as_ref or element_refs or something like that? Still unfortunate: I would like to use the same name as Option as the methods are so similar.

This comment has been hidden.

CryZe commented on Aug 14, 2020

JulianKnodt commented on Aug 14, 2020

Hm I was thinking about this, and maybe it's useful to define an intermediate struct, which is like an Iterator, but doesn't actually implement the iterator interface. When something like map or zip is called on it, it applies the intermediate functions on it and returns [T'; N]. That way, we could avoid creating many lang items and also allow for intermixing of different calls(possibly). Notably, I think not having enumerate makes a lot of array functions more clunky, because I imagine it's a common use case to use an index to initialize an element.

This comment was marked as off-topic.

scottmcm commented on Aug 14, 2020 •

maybe it's useful to define an intermediate struct, which is like an Iterator, but doesn't actually implement the iterator interface.

I don't think a separate type is needed here. As a parallel, map and cloned and such are available directly on Option, without needing to go through another type.

I imagine it's a common use case to use an index to initialize an element.

I agree, and made ArrayTools::indices to construct such an array directly.

Hopefully a future PR will allow us to consider just having that in core.

if anyone could help with the "new impl block lang item" problem

Any chance the where T: Copy can go on the method instead of the impl block?

JulianKnodt commented on Aug 14, 2020 •

Hm, if I can offer a counterpoint to Option offering map and cloned, Vec doesn't offer map directly, which to me is because it's expensive to perform map on larger vectors. I'm not sure if the common use case for [T; N] is closer to Option<T>, which would mean most operations are O(1) so be performed easily/directly, or for Vec, where operations such as map or clone are considered to take O(n) time.

CryZe commented on Aug 14, 2020 •

I'd actually say that Vec totally should have map as well, because it can potentially reuse its backing memory if the target type has the same size and alignment. So map not existing (yet?) on Vec is not really an argument.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

LukasKalbertodt commented on Aug 31, 2020

I decided to only deal with the as_ref/as_mut methods in this PR. I will create separate PRs for other methods. I changed the title, my top comment and hid a couple of comments as "off topic". I hope you don't mind me doing so in order to make this discussion more focused.

I just force pushed to name the methods as_ref_elements and as_mut_elements. It was suggested to name them as_[ref|mut]_array but I don't particularly like these names Those rather seem to indicate that they return a reference to the whole array. I am not completely happy with as_[ref|mut]_elements either though! If anyone has any good ideas, let us know!

bors commented on Sep 3, 2020

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

shepmaster commented on Sep 8, 2020

I would like to use the same name as Option as the methods are so similar.

I agree.

LukasKalbertodt commented on Sep 8, 2020

@shepmaster Are you suggesting we should try to run crater with as_ref/as_mut to see how much it really breaks?

shepmaster commented on Sep 8, 2020

run crater with as_ref/as_mut to see how much it really breaks

I think it's worth it. My gut says that the incidences will be much lower than array::into_iter because AsRef is mostly only used in generic contexts (fn foo(_: impl AsRef<...>)).

LukasKalbertodt commented on Sep 9, 2020

@shepmaster I hope you are right. I don't want to be the guy that always opens the "breaks all the code" PRs

I forced pushed to rename back to as_ref/as_mut. Crater queue luckily isn't too full right now.

LukasKalbertodt commented on Sep 9, 2020

@bors try

LukasKalbertodt commented 5 days ago

@dtolnay I'm not fully convinced yet, but we can still discuss the name before stabilization. I updated the PR with the names you suggested :)

rust-log-analyzer commented 5 days ago

The job mingw-check failed! Check out the build log: (web) (plain)

   Compiling libc v0.2.79
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.39
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error: the item `MaybeUninit` is imported redundantly
    |
18  | use crate::mem::MaybeUninit;
18  | use crate::mem::MaybeUninit;
    |     ----------------------- the item `MaybeUninit` is already imported here
485 |         use crate::mem::MaybeUninit;
    |             ^^^^^^^^^^^^^^^^^^^^^^^
    |
    |
    = note: `-D unused-imports` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `core`


To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:24

dtolnay commented 5 days ago

I pushed a rebase to fix the conflict with #79451.

@bors r+

bors commented 5 days ago

Commit 4038042 has been approved by dtolnay

LukasKalbertodt commented 5 days ago

Oh wow, you're fast :D I also rebased locally and was just testing. Thanks!

bors commented 5 days ago

Testing commit 4038042 with merge 6bb7313...

bors commented 5 days ago

Test timed out

rust-log-analyzer commented 5 days ago

A job failed! Check out the build log: (web) (plain)

dtolnay commented 5 days ago

@bors retry

bors commented 5 days ago

Testing commit 4038042 with merge c5eae56...

bors commented 4 days ago

Test successful - checks-actions
Approved by: dtolnay
Pushing c5eae56 to master...