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

Stabilize 128-bit integers 🎉 #49101

Merged
merged 15 commits into from
Mar 26, 2018
Merged

Stabilize 128-bit integers 🎉 #49101

merged 15 commits into from
Mar 26, 2018

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented Mar 17, 2018

cc #35118

EDIT: This should be merged only after the following have been merged:

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Mar 17, 2018
@mark-i-m
Copy link
Member Author

What does this tidy error mean?

tidy error: /checkout/src/libcore/num/mod.rs:3961: mismatches to previous in: ["stability level", "since", "tracking issue"]

@mark-i-m
Copy link
Member Author

I'm also getting this error:

   Compiling std_unicode v0.0.0 (file:///nobackup/rust/src/libstd_unicode)
error: this feature has been stable since 1.26.0. Attribute no longer needed
  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs:15:12
   |
15 | #![feature(i128_type)]
   |            ^^^^^^^^^
   |
note: lint level defined here
  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs:1:31
   |
1  | #![cfg_attr(not(stage0), deny(warnings))]
   |                               ^^^^^^^^
   = note: #[deny(stable_features)] implied by #[deny(warnings)]

error: aborting due to previous error

error: Could not compile `compiler_builtins`.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Mar 17, 2018

For the latter error, it looks like libcompiler_builtins/src/lib.rs of the std_unicode crate has #![feature(i128_type)] on line 15 (because it was unstable). Now that it's stable, it's erroring, because it also has #![deny(warnings)] and the compiler warns when you #![feature()] an already stable feature.

@PlasmaPower
Copy link
Contributor

Here's the line: https://github.com/rust-lang-nursery/compiler-builtins/blob/266ea0740a5bdd262a38bbd88fb55fc3d2a7a96e/src/lib.rs#L15

Unfortunately it's in a submodule (different repository) so I don't think there's a good way to simultaneously change them.

@@ -0,0 +1,18 @@
# `repri128`
Copy link
Contributor

Choose a reason for hiding this comment

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

s/repri128/repr128

@@ -261,10 +264,11 @@ If you're using a nightly version of rustc, just add the corresponding feature
to be able to use it:

```
#![feature(i128)]
#![feature(repri128)]
Copy link
Contributor

Choose a reason for hiding this comment

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

s/repri128/repr128

@@ -250,7 +250,10 @@ An unstable feature was used.
Erroneous code example:

```compile_fail,E658
let x = ::std::u128::MAX; // error: use of unstable library feature 'i128'
#[repr(u128)] // error: use of unstable library feature 'i128'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: language feature repr128

@@ -4,7 +4,7 @@ error[E0658]: use of unstable library feature 'i128' (see issue #35118)
LL | let _ = ::std::u128::MAX; //~ ERROR E0658
| ^^^^^^^^^^^^^^^^
|
= help: add #![feature(i128)] to the crate attributes to enable
= help: add #![feature(repri128)] to the crate attributes to enable
Copy link
Contributor

Choose a reason for hiding this comment

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

s/repri128/repr128

Copy link
Contributor

Choose a reason for hiding this comment

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

Er, shouldn't this test be testing another feature now :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the whole stderr needs to change... I will fix this one after I get the rest compiling successfully.

@@ -10,20 +10,20 @@

// gate-test-i128_type

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove the i128_type tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left this one as a test for the repr128 gate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it looks like there is already a test for that.

@@ -11,7 +11,7 @@
// Tests saturating float->int casts. See u128-as-f32.rs for the opposite direction.
// compile-flags: -Z saturating-float-casts

#![feature(test, i128, i128_type, stmt_expr_attributes)]
#![feature(test, i128, stmt_expr_attributes)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray feature(i128) left behind.

@@ -10,7 +10,7 @@

// ignore-emscripten u128 not supported

#![feature(test, i128, i128_type)]
#![feature(test, i128)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray feature(i128) left behind.

@goodmanjonathan
Copy link
Contributor

Regarding the first tidy failure, it looks like you forgot to change a few items to #[stable] starting on line 3961 of that file.

@@ -10,20 +10,20 @@

// gate-test-i128_type
Copy link
Member

Choose a reason for hiding this comment

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

You'll probably have to remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :) I removed the test altogether since there is already a test for the repr128 gate.

@petrochenkov
Copy link
Contributor

r? @nagisa or @alexcrichton

@rust-highfive rust-highfive assigned nagisa and unassigned petrochenkov Mar 17, 2018
@mark-i-m
Copy link
Member Author

Thanks @goodmanjonathan and @PlasmaPower ! I am fixing those things now :)

@mark-i-m
Copy link
Member Author

Hmm... I am still getting this tidy error:

tidy error: /nobackup/rust/src/libcore/num/mod.rs:3961: mismatches to previous in: ["stability level", "since", "tracking issue"]

But I fixed that line?

@mark-i-m
Copy link
Member Author

Re: #49101 (comment)

How do I actually fix this? Every time ./x.py updates submodules my dirty changes get reverted...

@PlasmaPower
Copy link
Contributor

I'd get the change on a different branch or the submodule's repository, then change the submodule's to point to that branch. I'm not sure if that's the right way to do this though.

@kennytm
Copy link
Member

kennytm commented Mar 17, 2018

@mark-i-m Add [build] submodules = false to your config.toml to disable automatic submodule update.

(Using __git_ps1 is highly recommended to visualize stale submodules after auto-update is disabled.)

@nagisa
Copy link
Member

nagisa commented Mar 17, 2018

I’ll review this after I get some sleep.

@mark-i-m
Copy link
Member Author

Travis build is now failing because of the compiler_builtins error. Should I make a PR to that repo?

@mark-i-m
Copy link
Member Author

Ok, I think the compiler_builtins error should be the last thing stopping this PR.

@mark-i-m mark-i-m changed the title [WIP] Stabilize 128-bit integers! Stabilize 128-bit integers 🎉 Mar 18, 2018
@mark-i-m
Copy link
Member Author

Made a PR on compiler_builtins

@mark-i-m
Copy link
Member Author

@nagisa Travis is passing again. nth try is the charm?

@kennytm
Copy link
Member

kennytm commented Mar 26, 2018

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Mar 26, 2018

📌 Commit 140bf94 has been approved by nagisa

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2018
@bors
Copy link
Contributor

bors commented Mar 26, 2018

⌛ Testing commit 140bf94 with merge 188e693...

bors added a commit that referenced this pull request Mar 26, 2018
Stabilize 128-bit integers 🎉

cc #35118

EDIT: This should be merged only after the following have been merged:
- [x] rust-lang/compiler-builtins#236
- [x] rust-lang/book#1230
@bors
Copy link
Contributor

bors commented Mar 26, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 188e693 to master...

@Tom-Phinney
Copy link

Tom-Phinney commented May 28, 2018

FWIW, here are additional files in rustc that as of 2018.05.12 did not handle usize == u128, which will be needed by RISC-V with RV128I option when LLVM or some other backend supports it. The grep string (between « and ») was «#[cfg(target_pointer_width = "(16|32|64|128)")]»

  • lib_alloc/vec_deque.rs
  • libcore/fmt/num.rs
  • libcore/iter/range.rs
  • libcore/num/mod.rs
  • libcore/num/wrapping.rs
  • libcore/tests/hash/sip.rs
  • libcore/tests/iter.rs
  • libcore/tests/mem.rs
  • librustc_data_structures/fx.rs
  • libserialize/leb128.rs
  • sys_common/backtrace.rs
  • test/compile-fail/huge-enum.rs
  • test/compile-fail/issue-17913.rs
  • test/compile-fail/int-exceeding-bitshifts2.rs
  • test/run-pass/const-negation.rs
  • test/run-pass/enum-discrim-autosizing.rs
  • test/run-pass/extern-types-pointer-cast.rs
  • test/run-pass/huge-largest-array.rs
  • test/run-pass/issue-2895.rs
  • test/run-pass/num-wrapping.rs
  • test/run-pass/struct-return.rs
  • test/run-pass/vec-fixed-length.rs
  • test/run-pass/wrapping_int_api.rs

@est31
Copy link
Member

est31 commented May 28, 2018

@Tom-Phinney it would be best to file an issue about this as inside this PR thread this is likely going to get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.