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

const_intrinsic_copy - Add Reference to tracking issue #80699

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Jan 4, 2021

Add reference to tracking issue #80697 for feature gate added in previous PR #79684

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Jan 4, 2021
#[inline]
pub const unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {
extern "rust-intrinsic" {
#[rustc_const_unstable(feature = "const_intrinsic_copy", issue = "none")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have thought it is, otherwise you should not be able to call it below, looks like we have a hole in our analysis somewhere. The rustc_const_unstable attribute is what makes the intrinsic const, since we cannot have const fn intrinsics since the syntax doesn't allow this.

Copy link
Contributor

Choose a reason for hiding this comment

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

uff.. I just checked, we have no unit tests ensuring that this actually works, so it must've regressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I noticed it was because I did not see the corresponding line for copy_nonoverlapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I got the error before the rebase(the reason the attribute was on copy at all)

Copy link
Contributor Author

@usbalbin usbalbin Jan 4, 2021

Choose a reason for hiding this comment

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

As far as I can tell, only looking at #79684, the error probably disappeared somewhere between the 4th and 21th of December. Quite a large span, but perhaps somewhere to start...

I might be completely wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

I... have an inlkling. I'm waiting for a rebuild with some more trace! instructions, now that I am able to reproduce. I think the issue may be that the copy intrinsic is declare inside a function with rustc_const_unstable and inherits that

@oli-obk
Copy link
Contributor

oli-obk commented Jan 4, 2021

@bors r+ Let's merge it even with the "bug", I'll start working on it immediately.

@bors
Copy link
Contributor

bors commented Jan 4, 2021

📌 Commit 63f5d61 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 Jan 4, 2021
@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 4, 2021

Ok, thanks :)

@bors
Copy link
Contributor

bors commented Jan 5, 2021

⌛ Testing commit 63f5d61 with merge 68ec332...

@bors
Copy link
Contributor

bors commented Jan 5, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 68ec332 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 5, 2021
@bors bors merged commit 68ec332 into rust-lang:master Jan 5, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 5, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 18, 2021
…sics, r=RalfJung

Stability oddity with const intrinsics

cc `@RalfJung`

In rust-lang#80699 (comment) `@usbalbin` realized we accepted some intrinsics as `const` without a `#[rustc_const_(un)stable]` attribute. I did some digging, and that example works because intrinsics inherit their stability from their parents... including `#[rustc_const_(un)stable]` attributes. While we may want to fix that (not sure, wasn't there just a MCPed PR that caused this on purpose?), we definitely want tests for it, thus this PR adding tests and some fun tracing statements.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2021
…cs, r=RalfJung

Stability oddity with const intrinsics

cc `@RalfJung`

In rust-lang#80699 (comment) `@usbalbin` realized we accepted some intrinsics as `const` without a `#[rustc_const_(un)stable]` attribute. I did some digging, and that example works because intrinsics inherit their stability from their parents... including `#[rustc_const_(un)stable]` attributes. While we may want to fix that (not sure, wasn't there just a MCPed PR that caused this on purpose?), we definitely want tests for it, thus this PR adding tests and some fun tracing statements.
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.

6 participants