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

Revert revert of constness in #86003 #86295

Merged
merged 11 commits into from
Jun 27, 2021

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Jun 14, 2021

Re-constify mem::swap, mem::replace, ptr::write which were marked as not const in #86003

Once the checks pass, this should solve #86236

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Jun 14, 2021
@rust-log-analyzer

This comment has been minimized.

@usbalbin usbalbin mentioned this pull request Jun 14, 2021
6 tasks
@usbalbin
Copy link
Contributor Author

Would it make sense to make a tracking issue for const_ptr_write?

@RalfJung
Copy link
Member

@pnkfelix any objections to this? I am not sure why these were de-constified to begin with, so just making sure I am not missing anything here. :)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

@usbalbin we also lost src/test/ui/consts/copy-intrinsic.rs in #86003; could you add that back as well?

@RalfJung
Copy link
Member

LGTM -- r=me, except that it'd be good to hear back from @pnkfelix. Let's give them a few more days.

r? @RalfJung

@rust-log-analyzer

This comment has been minimized.

@usbalbin
Copy link
Contributor Author

Sorry but I am not quite sure what to do with the errors (or rather lack of errors)

@usbalbin
Copy link
Contributor Author

Are those things that are no longer checked? Should I remove those tests or somehow make sure the errors are detected again?

@RalfJung
Copy link
Member

Something is strange, it looks like none of the errors showed up -- but the exit status is still 1...?

@RalfJung
Copy link
Member

RalfJung commented Jun 15, 2021

Ah, I think I know what happens: the errors now have the wrong span; they are shown with spans in the standard library.

To fix this, you should call the intrinsics directly in this test. You will probably need to add a extern "rust-intrinsic" block to import them.

@RalfJung
Copy link
Member

So, something like this. But at least on the playground this doesn't change the spans that are shown...

@usbalbin
Copy link
Contributor Author

Thanks :)

What should I do about

error: module has missing stability attribute
  --> src/main.rs:2:1

?
Same error in your playground

@RalfJung
Copy link
Member

Ah... well we'll need to add a stability attribute. This should do it:

#![stable(feature = "dummy", since = "1.0.0")]

@usbalbin
Copy link
Contributor Author

So if you write code which calls unstable code than you have to be explicit about the stabilitiy of your code?

Again thanks :)

Would you like me to squash the two latest commits (2faa57a and d1ae4e2) onto one?

@RalfJung
Copy link
Member

RalfJung commented Jun 15, 2021

A chain of things happens here:

  • To call copy_nonoverlapping, it needs to be const fn
  • To make an intrinsic const fn, we need to add rustc_const_unstable
  • To be allowed to use that attribute we need to enable the staged_api feature
  • Once we enable that feature we need to give stability attributes to everything; the easiest thing is to make is all stable so that we don't have to enable feature gates to call our own stuff. (Maybe we don't actually need that, and #![unstable(...)] would also work.

Would you like me to squash the two latest commits (2faa57a and d1ae4e2) onto one?

Seems fine to me to keep this separate.

@usbalbin
Copy link
Contributor Author

Ah, okay

@usbalbin usbalbin marked this pull request as ready for review June 15, 2021 19:06
@RalfJung
Copy link
Member

@pnkfelix confirmed that as long as we don't touch copy/copy_nonoverlapping, we should be good. So, let's (re-)land this. Thanks @usbalbin :)
@bors r+

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 27, 2021

💔 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 Jun 27, 2021
@RalfJung
Copy link
Member

Looks like #86194 changed the output of some tests added here.

@rust-log-analyzer

This comment has been minimized.

@usbalbin
Copy link
Contributor Author

@pnkfelix confirmed that as long as we don't touch copy/copy_nonoverlapping, we should be good. So, let's (re-)land this. Thanks @usbalbin :)
@bors r+

Thank you :)

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2021

📌 Commit 4aa1267 has been approved by RalfJung

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

bors commented Jun 27, 2021

⌛ Testing commit 4aa1267 with merge 49ba936...

@bors
Copy link
Contributor

bors commented Jun 27, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 49ba936 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 27, 2021
@bors bors merged commit 49ba936 into rust-lang:master Jun 27, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 27, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants