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

slice::fill: use T instead of generic arg #71165

Merged
merged 1 commit into from
May 3, 2020
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 15, 2020

implements #70758 (comment)

As the discussion in #70758 has shifted, I now use T instead of &T.

@lcnr
Copy link
Contributor Author

lcnr commented Apr 15, 2020

Looking at #70752, this method is expected to optimize to mem set, will check if this still works when passing a reference...

memset for &0, loop for &17: https://rust.godbolt.org/z/xurE4n

@lcnr
Copy link
Contributor Author

lcnr commented Apr 15, 2020

r? @yoshuawuyts

@yoshuawuyts
Copy link
Member

@lcnr I think there are some gotchas to always taking by reference; replied on the main thread: #70758 (comment)

@Dylan-DPC-zz
Copy link

r? @LukasKalbertodt

@Dylan-DPC-zz Dylan-DPC-zz added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2020
@lcnr lcnr changed the title slice::fill: use &T instead of generic arg slice::fill: use T instead of generic arg Apr 16, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Apr 16, 2020

This now uses T directly instead of &T

edit: tbh I slightly misinterpreted the current state of discussion and there is still some disagreement.
While I don't care either way we can probably wait a few days before merging this 🤷

@crlf0710 crlf0710 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 24, 2020
@crlf0710 crlf0710 added 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 May 2, 2020
@Dylan-DPC-zz
Copy link

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented May 2, 2020

Could you avoid cloning the last element and instead move value into it directly? See this code for how Vec::resize does it.

@lcnr
Copy link
Contributor Author

lcnr commented May 2, 2020

done

@Amanieu
Copy link
Member

Amanieu commented May 2, 2020

Can you check that the code still lowers to a memset?

@lcnr
Copy link
Contributor Author

lcnr commented May 2, 2020

Looks good

https://rust.godbolt.org/z/Gt7zPV

@Amanieu
Copy link
Member

Amanieu commented May 2, 2020

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2020

📌 Commit 902aa62 has been approved by Amanieu

@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 May 2, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 3, 2020
`slice::fill`: use `T` instead of generic arg

implements rust-lang#70758 (comment)

As the discussion in rust-lang#70758 has shifted, I now use `T` instead of `&T`.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#71165 (`slice::fill`: use `T` instead of generic arg)
 - rust-lang#71314 (Implement RFC 2523, `#[cfg(version(..))]`)
 - rust-lang#71542 (Implement `confusable_idents` lint.)
 - rust-lang#71806 (typo)
 - rust-lang#71813 (Decode qualifs for associated const defaults)

Failed merges:

r? @ghost
@bors bors merged commit 8cb8d9c into rust-lang:master May 3, 2020
@lcnr lcnr deleted the patch-2 branch May 3, 2020 12:12
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 20, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? `@m-ou-se`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 20, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? ``@m-ou-se``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 25, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? `@m-ou-se`
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-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.

7 participants