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 slice::array_chunks_mut #75021

Merged
merged 5 commits into from
Sep 12, 2020
Merged

Add slice::array_chunks_mut #75021

merged 5 commits into from
Sep 12, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Aug 1, 2020

This follows array_chunks from #74373 with a mutable version, array_chunks_mut. The implementation is identical apart from mutability. The new tests are adaptations of the chunks_exact_mut tests, plus an inference test like the one for array_chunks.

I reused the unstable feature array_chunks and tracking issue #74985, but I can separate that if desired.

r? @withoutboats
cc @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020
@cuviper
Copy link
Member Author

cuviper commented Aug 1, 2020

I'm willing to do reverse versions too -- array_rchunks[_mut]? That could be in this PR or a followup.

I also want to try array_windows, but I do think that deserves a separate feature and tracking issue.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

Can't say much about the aliasing stuff in array_chunks_mut, but the rest looks good.

Iirc @JulianKnodt wanted to look into implementing fn array_windows

@lcnr
Copy link
Contributor

lcnr commented Aug 1, 2020

I think rchunks might be better with a different feature gate and in a separate PR, don't really care that much though

@cuviper
Copy link
Member Author

cuviper commented Aug 1, 2020

Can't say much about the aliasing stuff in array_chunks_mut,

Yeah, I'm not certain that's the best way to avoid a mutable alias, but it seems simple enough.

Iirc @JulianKnodt wanted to look into implementing fn array_windows

OK, sure, I'll leave them to it.

@lcnr
Copy link
Contributor

lcnr commented Aug 1, 2020

Afaik mutable aliases are fine as long as the aliasing mutable reference isn't used anymore, i.e. there is no ABAB access.

Does not using that explicit scope cause MIRI to fail?

@cuviper
Copy link
Member Author

cuviper commented Aug 1, 2020

I thought the mere existence of aliases was a problem, but I could be wrong. I haven't tried with miri yet.

@cuviper
Copy link
Member Author

cuviper commented Aug 1, 2020

Does not using that explicit scope cause MIRI to fail?

AFAICT, miri is fine with it either way. Should I simplify it then?

@lcnr
Copy link
Contributor

lcnr commented Aug 2, 2020

i think so 😄

@Dylan-DPC-zz
Copy link

@withoutboats this is ready for review

@crlf0710 crlf0710 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020
@Dylan-DPC-zz
Copy link

r? @scottmcm

@scottmcm
Copy link
Member

scottmcm commented Sep 5, 2020

The code here looks right, and given that array_chunks exists, a _mut version seems totally reasonable.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 5, 2020

📌 Commit fca686f7e7da8fb971958b3928178cd229fa1620 has been approved by scottmcm

@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 Sep 5, 2020
@bors
Copy link
Contributor

bors commented Sep 5, 2020

⌛ Testing commit fca686f7e7da8fb971958b3928178cd229fa1620 with merge 321229650bb03b6b0edc8fd1f5056dc6fee9f7a9...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-freebsd of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
   Compiling profiler_builtins v0.0.0 (/checkout/library/profiler_builtins)
[RUSTC-TIMING] build_script_build test:false 0.320
[RUSTC-TIMING] build_script_build test:false 0.344
[RUSTC-TIMING] build_script_build test:false 0.513
error[E0407]: method `get_unchecked` is not a member of trait `TrustedRandomAccess`
     |
     |
6062 | /     unsafe fn get_unchecked(&mut self, i: usize) -> &'a mut [T; N] {
6063 | |         unsafe { self.iter.get_unchecked(i) }
6064 | |     }
     | |_____^ not a member of trait `TrustedRandomAccess`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0407`.
[RUSTC-TIMING] core test:false 8.219
[RUSTC-TIMING] core test:false 8.219
error: could not compile `core`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace profiler compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-freebsd --target x86_64-unknown-freebsd
Build completed unsuccessfully in 0:00:09
== clock drift check ==
  local time: Sat Sep  5 02:22:39 UTC 2020
  local time: Sat Sep  5 02:22:39 UTC 2020
  network time: Sat, 05 Sep 2020 02:22:39 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (4048) (node)
Terminate orphan process: pid (4057) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Sep 5, 2020

💔 Test failed - checks-actions

@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 Sep 5, 2020
@scottmcm
Copy link
Member

scottmcm commented Sep 5, 2020

Looks like this might be a real failure:

method `get_unchecked` is not a member of trait `TrustedRandomAccess`

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2020
@cuviper
Copy link
Member Author

cuviper commented Sep 5, 2020

Ah, needed to catch up with #73565 -- plus tidy seems picker about unsafe comments now...

@cuviper cuviper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2020
@scottmcm
Copy link
Member

scottmcm commented Sep 12, 2020

@bors r+

(Sorry for the delay here!)

@bors
Copy link
Contributor

bors commented Sep 12, 2020

📌 Commit 86b9f71 has been approved by scottmcm

@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 Sep 12, 2020
@rust-lang rust-lang deleted a comment from bors Sep 12, 2020
@rust-lang rust-lang deleted a comment from bors Sep 12, 2020
@bors
Copy link
Contributor

bors commented Sep 12, 2020

⌛ Testing commit 86b9f71 with merge 8b6838b...

@bors
Copy link
Contributor

bors commented Sep 12, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: scottmcm
Pushing 8b6838b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2020
@bors bors merged commit 8b6838b into rust-lang:master Sep 12, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 12, 2020
@cuviper cuviper deleted the array_chunks_mut branch September 21, 2021 16:44
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.

10 participants