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 [T; N]::each_ref and [T; N]::each_mut #75490

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Aug 13, 2020

This PR adds the methods each_ref and each_mut to [T; N]. The ability to add methods to arrays was added in #75212. These two methods are particularly useful with map which was also added in that PR. Tracking issue: #76118

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

@LukasKalbertodt

This comment has been minimized.

@LukasKalbertodt
Copy link
Member Author

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.

@LukasKalbertodt

This comment has been minimized.

@CryZe
Copy link
Contributor

CryZe commented Aug 13, 2020

@scottmcm's arraytools calls them as_[mut/ref]_array

@JulianKnodt
Copy link
Contributor

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.

@JulianKnodt

This comment has been minimized.

@scottmcm
Copy link
Member

scottmcm commented Aug 13, 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
Copy link
Contributor

JulianKnodt commented Aug 13, 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
Copy link
Contributor

CryZe commented Aug 13, 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.

@lcnr

This comment has been minimized.

@LukasKalbertodt

This comment has been minimized.

@lcnr

This comment has been minimized.

@jplatte

This comment has been minimized.

@LukasKalbertodt LukasKalbertodt changed the title Add [T; N]::as_ref and [T; N]::as_mut Add [T; N]::as_ref_elements and [T; N]::as_mut_elements Aug 30, 2020
@LukasKalbertodt
Copy link
Member Author

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!

/// ```
///
/// This method is particularly useful if combined with other methods, like
/// [`map`](#method.map). This way, you can can avoid moving the original
Copy link
Member Author

Choose a reason for hiding this comment

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

Does anyone know how to use intra-doc links for this? [`map`] and [`Self::map`] don't work.

@bors
Copy link
Contributor

bors commented Sep 3, 2020

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 3, 2020
@LukasKalbertodt LukasKalbertodt force-pushed the add-basic-array-methods branch 2 times, most recently from 2037828 to 2a1b534 Compare September 3, 2020 07:14
@shepmaster
Copy link
Member

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

I agree.

@LukasKalbertodt
Copy link
Member Author

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

@shepmaster
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

@bors try

@LukasKalbertodt LukasKalbertodt changed the title Add [T; N]::as_ref_elements and [T; N]::as_mut_elements Add [T; N]::each_ref and [T; N]::each_mut Jan 11, 2021
@LukasKalbertodt
Copy link
Member Author

@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
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
   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

These methods work very similarly to `Option`'s methods `as_ref` and
`as_mut`. They are useful in several situation, particularly when
calling other array methods (like `map`) on the result. Unfortunately,
we can't easily call them `as_ref` and `as_mut` as that would shadow
those methods on slices, thus being a breaking change (that is likely
to affect a lot of code).
@dtolnay
Copy link
Member

dtolnay commented Jan 11, 2021

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

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2021

📌 Commit 4038042 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 11, 2021
@dtolnay dtolnay mentioned this pull request Jan 11, 2021
5 tasks
@LukasKalbertodt
Copy link
Member Author

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

@bors
Copy link
Contributor

bors commented Jan 11, 2021

⌛ Testing commit 4038042 with merge 6bb7313d5c346ae7f2397e6d61390ff288e32016...

@bors
Copy link
Contributor

bors commented Jan 11, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2021
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)

@dtolnay
Copy link
Member

dtolnay commented Jan 11, 2021

@bors retry

@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 11, 2021
@bors
Copy link
Contributor

bors commented Jan 11, 2021

⌛ Testing commit 4038042 with merge c5eae56...

@bors
Copy link
Contributor

bors commented Jan 11, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 11, 2021
@bors bors merged commit c5eae56 into rust-lang:master Jan 11, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 11, 2021
@LukasKalbertodt LukasKalbertodt deleted the add-basic-array-methods branch January 11, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.