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

Introduce rustc_const_stable and explain rustc_const_unstable #542

Merged
merged 1 commit into from
Dec 24, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 23, 2019

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

I don't have r+ here, but 👍

stable function as `const fn` without instantly stabilizing the `const fn`ness.

Furthermore this attribute is needed to mark an intrinsic as `const fn`, because
there's no way to add `const` to functions in `extern` blocks for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

argh... this is a syntactic problem? (I should fix this as part of my item-parsing unification work...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... it's more of a const is not a part of https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.FnDecl.html

But since we have safe intrinsics nowadays, and const intrinsics, maybe we really should move to using a FnHeader. Though we don't want this as a general thing for extern items. The rust-intrinsic ABI really should go away and we should have a better scheme for declaring intrinsics. I think @eddyb had some thoughts on this, not sure if those were ever written down, but maybe we have an issue about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

well... it's more of a const is not a part of https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.FnDecl.html

It should be an FnSig syntactically in ForeignItemKind. There should just be one function item grammar (aside from the snafu of requiring parameter names sometimes and sometimes not but that's a minor issue controlled by a flag basically).

Though I think you're about the semantic issue... we wanted #[rustc_intrinsic] iirc and there's an issue somewhere about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be an FnSig syntactically in ForeignItemKind.

That won't work out of the box because the default for extern functions is unsafe, so you'd still need parser hacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need parser hacks. If we want to prevent anything, e.g. async fn in extern, then we can do it in ast_validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not about denying, it's that extern functions are unsafe by default, so the normal parser will mark them safe

Copy link
Contributor

Choose a reason for hiding this comment

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

That's something easily fixed in lowering which is the right place to draw such semantic distinctions.

@Centril Centril merged commit c461c85 into master Dec 24, 2019
@Centril Centril deleted the oli-obk-patch-1 branch December 24, 2019 00:16
mark-i-m pushed a commit to spastorino/rustc-dev-guide that referenced this pull request Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants