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

Mark appropriate const fns as promotable #471

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 11, 2018

This is in preparation for doing the same change to rustc. The feature and attribute don't exist yet, so there'll be some warnings, but they'll go away once the actual change hits nightly

@lu-zero
Copy link
Contributor

lu-zero commented Jun 11, 2018

Could you please add a link to the rustc commit? :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

Can't, the PR isn't even open yet ;) I'll do that when I get there.

Could you place this PR into a branch so we're following the rustc rules for lockstep upgrades correctly?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

And now we have a PR: rust-lang/rust#51500

@lu-zero
Copy link
Contributor

lu-zero commented Jun 11, 2018

I'd ask @alexcrichton and @gnzlbg to do it since I never did before :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

git fetch origin pull/471/head:oli-obk-lockstep
git checkout oli-obk-lockstep
git push origin oli-obk-lockstep

@lu-zero
Copy link
Contributor

lu-zero commented Jun 11, 2018

That part I knew, the branch naming and where to put it is the part I'm completely oblivious of :)

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 11, 2018

It might be obvious, but what is a promotable const fn (neither the commit msgs here nor in rustc upstream explain)? Also, it would be cool to add some tests that would break without the promotable attribute (you can add them inline, or in stdsimd/crates/coresimd/tests).

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

A promotable const fn is one that is marked with the #[promotable_const_fn] attribute. Currently that means it can only call other promotable const fns and it cannot contain unions. This step is done as an intermediate solution for various fears we have around promoting calls to const fn and the attribute will likely never become stable, but be replaced by an automatic analysis.

We have such tests in the rustc repository, I'll check if you have compile-fail tests, if not, this is untestable anyway,

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 11, 2018

So does that mean that all const fns that do not contain unions and only call other promotable const fns should use the attribute? Some of the functions you added the attribute to are private (test_bit, set_bit, etc.), should it be appleid to all const fns that can use it, or only those in the public API of std ?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

So does that mean that all const fns that do not contain unions and only call other promotable const fns should use the attribute?

No further functions should need this attribute. These two PRs just marked all the const fns callable from stable code (directly or indirectly) as promotable (because they already were stable). Now we can continue stabilizing const fns without stabilizing their promotability at the same time

@@ -11,25 +11,29 @@ macro_rules! impl_mask_minimal {
/// Creates a new instance with each vector elements initialized
/// with the provided values.
#[inline]
#[promotable_const_fn]
pub const fn new($($elem_name: bool),*) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

which stable code can call the const functions in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to find that out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh... other than removing the attribute and following the fallout?

Copy link
Contributor

@gnzlbg gnzlbg Jun 11, 2018

Choose a reason for hiding this comment

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

Could you post the fallout? I am curious because nothing in this module should be reachable from stable code and IIRC nothing in the stable API of the crate uses it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, only the stdsimd/arch/detect/cache.rs functions should be reachable from stable code =/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... will do, I might have been a little overeager in marking things here, because it was hard to tell what was called from where.

Copy link
Contributor

@gnzlbg gnzlbg Jun 11, 2018

Choose a reason for hiding this comment

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

Might be that I screwed up. Worst case we can just merge this as is, and once the next nightly I can try removing the attribute from all places where it shouldn't be needed. Let me know when you want this merged (probably after the PR upstream is reviewed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed all unnecessary attributes

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 11, 2018

I'll check if you have compile-fail tests

I have not found any such tests. So we'd have to add such tests, which is a lot of effort for little gain, as it's tested by the rustc test suite and will be checked here once the attribute is part of rustc

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 22, 2018

We're going an alternative route, so no attributes are needed

@oli-obk oli-obk closed this Jun 22, 2018
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.

3 participants