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

Stablilize const_int_{rotate,wrapping,sign} #57105

Closed
wants to merge 9 commits into
base: master
from

Conversation

@Centril
Copy link
Contributor

Centril commented Dec 24, 2018

This PR intends to stabilize the following stable methods as const fns:

  • const_int_rotate
    • rotate_left
    • rotate_right
  • const_int_wrapping
    • wrapping_add
    • wrapping_sub
    • wrapping_mul
    • wrapping_shl
    • wrapping_shr
  • const_int_sign
    • is_positive
    • is_negative

(this is the current set; and may be reduced during review...)

r? @oli-obk
(please review this until you think its in a ready state for team review; but don't r+)

cc @RalfJung wrt. state of miri re. the aforementioned methods.
cc #53718

I've added T-lang since this affects intrinsics and the operational semantics of Rust's const fn fragment.
Once @oli-obk thinks its ready for team review I'll fcp-merge.

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 24, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:217eafc2:start=1545677548739262030,finish=1545677602213678815,duration=53474416785
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:57:24] .................................................................................................... 100/2928
[00:57:34] .................................................................................i.................. 200/2928
[00:57:42] .................................................................................................... 300/2928
[00:57:52] .................................................................................................... 400/2928
[00:58:01] ..........................FF.F...................................................................... 500/2928
[00:58:26] .................................................................................................... 700/2928
[00:58:37] .................................................................................................... 800/2928
[00:58:46] .................................................................................................... 900/2928
[00:59:00] .................................................................................................... 1000/2928
---
[01:02:40] .................................................................................................... 2600/2928
[01:02:49] .................................................................................................... 2700/2928
[01:02:59] .................................................................................................... 2800/2928
[01:03:10] .................................................................................................... 2900/2928
bj/build/x86_64-unknown-linux-gnu/test/run-pass/const-int-sign/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/const-int-sign/auxiliary"
[01:03:14] ------------------------------------------
[01:03:14] 
[01:03:14] ------------------------------------------
[01:03:14] stderr:
[01:03:14] stderr:
[01:03:14] ------------------------------------------
[01:03:14] {"message":"unknown feature `const_int_sign`","code":{"code":"E0635","explanation":"\nThe `#![feature]` attribute specified an unknown feature.\n\nErroneous code example:\n\n```compile_fail,E0635\n#![feature(nonexistent_rust_feature)] // error: unknown feature\n```\n\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/run-pass/const-int-sign.rs","byte_start":478,"byte_end":492,"line_start":11,"line_end":11,"column_start":12,"column_end":26,"is_primary":true,"text":[{"text":"#![feature(const_int_sign)]","highlight_start":12,"highlight_end":26}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0635]: unknown feature `const_int_sign`\n  --> /checkout/src/test/run-pass/const-int-sign.rs:11:12\n   |\nLL | #![feature(const_int_sign)]\n   |            ^^^^^^^^^^^^^^\n\n"}
[01:03:14] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[01:03:14] {"message":"For more information about this error, try `rustc --explain E0635`.","code":null,"level":"","spanout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:03:14] 
[01:03:14] 
[01:03:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:03:14] Build completed unsuccessfully in 0:09:42
[01:03:14] Build completed unsuccessfully in 0:09:42
[01:03:14] Makefile:58: recipe for target 'check' failed
[01:03:14] make: *** [check] Error 1
4278492 .
2645360 ./obj
2632608 ./obj/build
1972796 ./obj/build/x86_64-unknown-linux-gnu
---
150156 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
150152 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
147760 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps
144216 ./obj/build/bootstrap/debug/incremental/bootstrap-2x7szixskz2uj
144212 ./obj/build/bootstrap/debug/incremental/bootstrap-2x7szixskz2uj/s-f7wajigbwr-1xzab29-c95hburtu2sh

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 25, 2018

@rfcbot fcp merge

Given @oli-obk's approval, I propose that the following set of stable methods should stabilize as const fns given that they are about pure arithmetic and thus there's no world in which we would want to turn this back:

  • const_int_rotate
    • rotate_left
    • rotate_right
  • const_int_wrapping
    • wrapping_add
    • wrapping_sub
    • wrapping_mul
    • wrapping_shl
    • wrapping_shr
  • const_int_sign
    • is_positive
    • is_negative
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 25, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 25, 2018

Why not include const_int_ops and const_int_conversion as well?

            pub const fn count_ones(self) -> u32 { (self as $UnsignedT).count_ones() }
            pub const fn count_zeros(self) -> u32 {
            pub const fn leading_zeros(self) -> u32 {
            pub const fn trailing_zeros(self) -> u32 {
            pub const fn swap_bytes(self) -> Self {
            pub const fn reverse_bits(self) -> Self {
            pub const fn from_be(x: Self) -> Self {
            pub const fn from_le(x: Self) -> Self {
            pub const fn to_be(self) -> Self { // or not to be?
            pub const fn to_le(self) -> Self {
            pub const fn to_be_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn to_le_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn to_ne_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn from_be_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
            pub const fn from_le_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
            pub const fn from_ne_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
            pub const fn count_ones(self) -> u32 {
            pub const fn count_zeros(self) -> u32 {
            pub const fn leading_zeros(self) -> u32 {
            pub const fn trailing_zeros(self) -> u32 {
            pub const fn swap_bytes(self) -> Self {
            pub const fn reverse_bits(self) -> Self {
            pub const fn from_be(x: Self) -> Self {
            pub const fn from_le(x: Self) -> Self {
            pub const fn to_be(self) -> Self { // or not to be?
            pub const fn to_le(self) -> Self {
            pub const fn to_be_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn to_le_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn to_ne_bytes(self) -> [u8; mem::size_of::<Self>()] {
            pub const fn from_be_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
            pub const fn from_le_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
            pub const fn from_ne_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 25, 2018

@SimonSapin Hmm; I think I the reason I didn't go for overflowing_ was that they required let bindings which we don't have yet but that's a different feature gate... and then there was something about transmute which I didn't feel comfortable whitelisting since it opens up for un-const operations...

I'll see how much I can expand the set of methods to stably constify without whitelisting transmute... :)

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 26, 2018

This adds another unsafe power to const fn: Calling unchecked_shr/shl. However, these intrinsics are unstable, and they are only safely exposed exposed.

That seems fine to me, but do we have tests that check that these intrinsics correctly trigger a CTFE error when called with invalid arguments?

With such tests present, the set of functions proposed right now seems fine to me.

@SimonSapin from a review perspective, IMO it makes sense to have several small stabilizations instead of one big one. As long as we land them all in the same cycle, that still gives a coherent story for the release notes.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 26, 2018

@RalfJung

This adds another unsafe power to const fn: Calling unchecked_shr/shl. However, these intrinsics are unstable, and they are only safely exposed exposed.

That seems fine to me, but do we have tests that check that these intrinsics correctly trigger a CTFE error when called with invalid arguments?

Great point; I'm not sure; I'll check if we do (or add them)... and in the interim:

@rfcbot concern check-tests-for-unchecked_shrl

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 26, 2018

☔️ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts.

@Centril Centril force-pushed the Centril:stabilizations_const_ints branch from 6cb7bac to d426575 Dec 29, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 29, 2018

@RalfJung Some tests already existed, but I've extended them in this PR to be more thorough.

@rfcbot resolve check-tests-for-unchecked_shrl

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 29, 2018

@oli-obk Can you recheck the PR given recent changes?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 29, 2018

I think we should make the overflowing and rotate intrinsics safe just like we did with size_of and align_of

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2018

I've extended them in this PR to be more thorough.

Thanks, that looks great!

@Centril Centril force-pushed the Centril:stabilizations_const_ints branch from d426575 to 50152d2 Dec 31, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 31, 2018

@oli-obk

I think we should make the overflowing and rotate intrinsics safe just like we did with size_of and align_of

Done in fedfb61 and used in 50152d2.

Show resolved Hide resolved src/libcore/num/mod.rs Outdated
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 31, 2018

Just a style nit otherwise everything looks great

@Lokathor

This comment has been minimized.

Copy link

Lokathor commented Jan 5, 2019

wrapping_neg and overflowing_neg can also be made totally branchless, and thus const, with a little bit of bit fiddling

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6a800d9c97885c1aba464ea3103c86e6

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 6, 2019

wrapping_neg and overflowing_neg can also be made totally branchless, and thus const, with a little bit of bit fiddling

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6a800d9c97885c1aba464ea3103c86e6

Let's discuss this in a PR or another issue if you want; it's sorta off-topic for this PR.

@Lokathor

This comment has been minimized.

Copy link

Lokathor commented Jan 6, 2019

Can do, I'll open an issue for it.

EDIT: actually I'll open an issue once this PR gets merged, since those depend on this being merged

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 8, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2019

Rollup merge of rust-lang#57234 - Centril:const-stabilizations-2, r=o…
…li-obk

Const-stabilize `const_int_ops` + `const_ip`

r? @oli-obk

I've added T-lang since this affects intrinsics and the operational semantics of Rust's `const fn` fragment.
This PR depends on rust-lang#57105 but the FCP intent does not.

## Stable APIs proposed for constification

+ `const_int_ops`:
    + `count_ones`
    + `count_zeros`
    + `leading_zeros`
    + `trailing_zeros`
    + `swap_bytes`
    + `from_be`
    + `from_le`
    + `to_be`
    + `to_le`
+ `const_ip`
    + `Ipv4Addr::new`

## Unstable APIs constified

+ `const_int_conversion`:
    + `reverse_bits`

Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2019

Rollup merge of rust-lang#57234 - Centril:const-stabilizations-2, r=o…
…li-obk

Const-stabilize `const_int_ops` + `const_ip`

r? @oli-obk

I've added T-lang since this affects intrinsics and the operational semantics of Rust's `const fn` fragment.
This PR depends on rust-lang#57105 but the FCP intent does not.

## Stable APIs proposed for constification

+ `const_int_ops`:
    + `count_ones`
    + `count_zeros`
    + `leading_zeros`
    + `trailing_zeros`
    + `swap_bytes`
    + `from_be`
    + `from_le`
    + `to_be`
    + `to_le`
+ `const_ip`
    + `Ipv4Addr::new`

## Unstable APIs constified

+ `const_int_conversion`:
    + `reverse_bits`

bors added a commit that referenced this pull request Jan 11, 2019

Auto merge of #57234 - Centril:const-stabilizations-2, r=oli-obk
Const-stabilize `const_int_ops` + `const_ip`

r? @oli-obk

Note for relnotes: This PR includes #57105.

I've added T-lang since this affects intrinsics and the operational semantics of Rust's `const fn` fragment.

## Stable APIs proposed for constification

+ `const_int_ops`:
    + `count_ones`
    + `count_zeros`
    + `leading_zeros`
    + `trailing_zeros`
    + `swap_bytes`
    + `from_be`
    + `from_le`
    + `to_be`
    + `to_le`
+ `const_ip`
    + `Ipv4Addr::new`

## Unstable APIs constified

+ `const_int_conversion`:
    + `reverse_bits`

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #57234 - Centril:const-stabilizations-2, r=oli-obk
Const-stabilize `const_int_ops` + `const_ip`

r? @oli-obk

## Note for relnotes: This PR includes #57105.

I've added T-lang since this affects intrinsics and the operational semantics of Rust's `const fn` fragment.

## Stable APIs proposed for constification

+ `const_int_ops`:
    + `count_ones`
    + `count_zeros`
    + `leading_zeros`
    + `trailing_zeros`
    + `swap_bytes`
    + `from_be`
    + `from_le`
    + `to_be`
    + `to_le`
+ `const_ip`
    + `Ipv4Addr::new`

## Unstable APIs constified

+ `const_int_conversion`:
    + `reverse_bits`
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 12, 2019

This is subsumed by #57234 (this didn't get closed due to the last commit, but that isn't important...). Since this is done I'll go ahead and close it out.

@Centril Centril closed this Jan 12, 2019

@Centril Centril deleted the Centril:stabilizations_const_ints branch Jan 12, 2019

@Centril Centril removed the T-lang label Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment