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

Constify copy related functions #83091

Merged
merged 4 commits into from
Mar 16, 2021
Merged

Constify copy related functions #83091

merged 4 commits into from
Mar 16, 2021

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Mar 13, 2021

Constify

  • *const T::copy_to[_nonoverlapping]
  • *mut T::copy_to[_nonoverlapping]
  • *mut T::copy_from[_nonoverlapping]
  • mem::transmute_copy
  • mem::swap
  • ptr::swap[_nonoverlapping]
  • mem::replace
  • ptr::replace

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021
@usbalbin usbalbin mentioned this pull request Mar 13, 2021
3 tasks
@usbalbin
Copy link
Contributor Author

usbalbin commented Mar 13, 2021

What do we want to call the feature gate for copy_to and copy_from? Would it make sense to reuse const_intrinsic_copy since copy_[to/from] are not much more than wrappers for the functions behind that?

@RalfJung
Copy link
Member

Cc @rust-lang/wg-const-eval

What do we want to call the feature gate for copy_to and copy_from? Would it make sense to reuse const_intrinsic_copy since copy_[to/from] are not much more than wrappers for the functions behind that?

Yeah that makes sense.

#[rustc_const_unstable(feature = "const_swap", issue = "none")]
pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize) {
// FIXME: Is it okay to remove these checks? The same checks were removed for
// ptr::copy_nonoverlapping
Copy link
Contributor Author

@usbalbin usbalbin Mar 14, 2021

Choose a reason for hiding this comment

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

Do we want to keep these checks? The same checks were removed for ptr::copy_nonoverlapping when it was constified in #79684

Copy link
Member

Choose a reason for hiding this comment

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

For copy_nonoverlapping we ended up removing them entirely also for performance reasons. I am not sure if those apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Not sure" as in "Lets undo those changes for now so we can continue with the less controversial changes and maybe consider that for some other time"? Or just "Not sure" :)

Copy link
Member

@RalfJung RalfJung Mar 15, 2021

Choose a reason for hiding this comment

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

"Not sure" as in "let's see what others think"... @oli-obk ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm for removing the checks. I don't see why swap_nonoverlapping should have them while copy_nonoverlapping doesn't have them.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fair.

@usbalbin
Copy link
Contributor Author

I have removed the // FIXME mentioned above and squashed some commits. Do you want me to squash all commits into one or keep it as it is, one per feature gate?

@usbalbin usbalbin marked this pull request as ready for review March 15, 2021 17:27
@@ -394,7 +394,8 @@ pub const fn slice_from_raw_parts_mut<T>(data: *mut T, len: usize) -> *mut [T] {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn swap<T>(x: *mut T, y: *mut T) {
#[rustc_const_unstable(feature = "const_swap", issue = "none")]
Copy link
Contributor

Choose a reason for hiding this comment

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

could you create and reference an issue for these?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2021

Do you want me to squash all commits into one or keep it as it is, one per feature gate?

one commit per feature gate is great, thanks!

Once all unstable feature attributes have an issue instead of none, r=me

@usbalbin
Copy link
Contributor Author

@oli-obk

Once all unstable feature attributes have an issue instead of none, r=me

Ok, so...

@bors r=oli-obk

...I hope that is right :)

@bors
Copy link
Contributor

bors commented Mar 15, 2021

@usbalbin: 🔑 Insufficient privileges: Not in reviewers

@usbalbin usbalbin changed the title constify copy_to and copy_from Constify copy related functions Mar 15, 2021
@joshtriplett
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Mar 16, 2021

📌 Commit db9a53b has been approved by oli-obk

@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 Mar 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81822 (Added `try_exists()` method to `std::path::Path`)
 - rust-lang#83072 (Update `Vec` docs)
 - rust-lang#83077 (rustdoc: reduce GC work during search)
 - rust-lang#83091 (Constify `copy` related functions)
 - rust-lang#83156 (Fall-back to sans-serif if Arial is not available)
 - rust-lang#83157 (No background for code in portability snippets)
 - rust-lang#83160 (Deprecate RustcEncodable and RustcDecodable.)
 - rust-lang#83162 (Specify *.woff2 files as binary)
 - rust-lang#83172 (More informative diagnotic from `x.py test` attempt atop beta checkout)
 - rust-lang#83196 (Use delay_span_bug instead of panic in layout_scalar_valid_range)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d3460cd into rust-lang:master Mar 16, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2021
…Mark-Simulacrum

Make copy/copy_nonoverlapping fn's again

Make copy/copy_nonoverlapping fn's again, rather than intrinsics.

This a short-term change to address issue rust-lang#84297.

It effectively reverts PRs rust-lang#81167 rust-lang#81238 (and part of rust-lang#82967), rust-lang#83091, and parts of rust-lang#79684.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 17, 2021
…Mark-Simulacrum

Make copy/copy_nonoverlapping fn's again

Make copy/copy_nonoverlapping fn's again, rather than intrinsics.

This a short-term change to address issue rust-lang#84297.

It effectively reverts PRs rust-lang#81167 rust-lang#81238 (and part of rust-lang#82967), rust-lang#83091, and parts of rust-lang#79684.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants