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

Add FromStr impl for NonZero types #58717

Merged
merged 6 commits into from Mar 29, 2019

Conversation

@hellow554
Copy link
Contributor

commented Feb 25, 2019

This is a WIP implementation because I do have some questions regarding the solution.

Somebody should ping the lang team on this I guess.
Please see the annotations on the code for more details.

Closes #58604

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@Centril

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@hellow554

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Just as a note: I'm on vacation the next two weeks, so I would really love to see some feedback soon @SimonSapin

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

#[unstable] unfortunately doesn’t work on trait impls. Since both the type and trait are stable, the impl will be immediately usable.

@rfcbot fcp merge

Please see the annotations on the code for more details.

I couldn’t find those.

@SimonSapin SimonSapin added the T-libs label Feb 26, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Feb 26, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

Show resolved Hide resolved src/libcore/num/mod.rs Outdated
@rfcbot

This comment has been minimized.

Copy link

commented Feb 26, 2019

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

@hellow554

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@SimonSapin I mean the github review thing regarding the unstable thing and from_str_radix.

So for the unstable attribute, I can change that to stable with version 1.34?
What about from_str_radix?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I don’t seem to find any review or comment about from_str_radix except this one 7 minutes ago. Maybe you still need to submit it?

I think that Beta 1.34 was or will be branched this week, so this should likely be 1.35. And yes, changed to stable.

Show resolved Hide resolved src/libcore/num/mod.rs Outdated
Show resolved Hide resolved src/libcore/num/mod.rs Outdated
Show resolved Hide resolved src/libcore/num/mod.rs
Show resolved Hide resolved src/libcore/num/mod.rs Outdated
@hellow554

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

TIL: you don't see that rewiew until I hit that button ^^ Sorry!

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@rust-lang/libs What do you think of adding a Zero variant to IntErrorKind (which is #[non_exhaustive] and unstable) and reuse it and ParseIntError for these conversions, rather than introducing new types?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@SimonSapin that strategy sounds plausible to me!

@hellow554 hellow554 marked this pull request as ready for review Feb 27, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Alright, diff look good. r+ when FCP finishes.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2019

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:14fff919:start=1553762208780875728,finish=1553762281039705083,duration=72258829355
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:25:01]    Compiling memmap v0.6.2
[00:25:06] error[E0308]: mismatched types
[00:25:06]     --> src/librustc_codegen_llvm/debuginfo/metadata.rs:1372:57
[00:25:06]      |
[00:25:06] 1372 |                             let value = truncate(value, niche.value.size(cx).bits());
[00:25:06]      |
[00:25:06]      = note: expected type `rustc_target::abi::Size`
[00:25:06]                 found type `u64`
[00:25:06] 
---
travis_fold:end:after_failure.1
travis_fold:start:after_failure.2
travis_time:start:0744391d
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access '/home/travis/Library/Logs/Diagnosile or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:26f26b8a
$ dmesg | grep -i kill

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)

fixed shift overflow
Fix according to oli-obk
(#58717 (comment))

@hellow554 hellow554 force-pushed the hellow554:nonzero_parse branch from 4d33d57 to 8f3e862 Mar 28, 2019

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

📌 Commit 8f3e862 has been approved by oli-obk

Centril added a commit to Centril/rust that referenced this pull request Mar 28, 2019

Rollup merge of rust-lang#58717 - hellow554:nonzero_parse, r=oli-obk
Add FromStr impl for NonZero types

This is a WIP implementation because I do have some questions regarding the solution.

Somebody should ping the lang team on this I guess.
Please see the annotations on the code for more details.

Closes rust-lang#58604

bors added a commit that referenced this pull request Mar 28, 2019

Auto merge of #59487 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #58717 (Add FromStr impl for NonZero types)
 - #59091 (Combine input and eval_always query types)
 - #59216 (Type dependent defs wrappers)
 - #59318 (rustc: Update linker flavor inference from filename)
 - #59320 (rustc: Allow using `clang` for wasm32 targets)
 - #59363 (#59361 Moved rustc edition opt to short list)
 - #59371 (ffi: rename VaList::copy to VaList::with_copy)
 - #59398 (Add a way to track Rustfix UI test coverage)
 - #59408 (compiletest: make path normalization smarter)
 - #59429 (When moving out of a for loop head, suggest borrowing it in nll mode)

Failed merges:

r? @ghost
@cuviper

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@hellow554

I don't see how "this is my fault" and therefore I don't know exactly what and how to fix it.

Sorry, I was trying to say it wasn't your fault, but I was too oblique about it. Your change triggered a latent compiler bug, so someone has to fix that (here or elsewhere) before you'll pass CI. Looks like you've got it sorted out now though.

@hellow554

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

I didn't see your comment as an attack, so please don't get me wrong. It was okay, that you said it and I see why. I was just overchallenged, because I never touched that code. But thankfully oli-obk helped me (danke!). I hope this will pass CI now. Thanks again!

bors added a commit that referenced this pull request Mar 28, 2019

Auto merge of #59487 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #58717 (Add FromStr impl for NonZero types)
 - #59091 (Combine input and eval_always query types)
 - #59216 (Type dependent defs wrappers)
 - #59318 (rustc: Update linker flavor inference from filename)
 - #59320 (rustc: Allow using `clang` for wasm32 targets)
 - #59363 (#59361 Moved rustc edition opt to short list)
 - #59371 (ffi: rename VaList::copy to VaList::with_copy)
 - #59398 (Add a way to track Rustfix UI test coverage)
 - #59408 (compiletest: make path normalization smarter)
 - #59429 (When moving out of a for loop head, suggest borrowing it in nll mode)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Mar 28, 2019

Auto merge of #59487 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #58717 (Add FromStr impl for NonZero types)
 - #59091 (Combine input and eval_always query types)
 - #59216 (Type dependent defs wrappers)
 - #59318 (rustc: Update linker flavor inference from filename)
 - #59320 (rustc: Allow using `clang` for wasm32 targets)
 - #59363 (#59361 Moved rustc edition opt to short list)
 - #59371 (ffi: rename VaList::copy to VaList::with_copy)
 - #59398 (Add a way to track Rustfix UI test coverage)
 - #59408 (compiletest: make path normalization smarter)
 - #59429 (When moving out of a for loop head, suggest borrowing it in nll mode)

Failed merges:

r? @ghost
@@ -1366,7 +1367,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
let value = (i.as_u32() as u128)
.wrapping_sub(niche_variants.start().as_u32() as u128)
.wrapping_add(niche_start);
let value = value & ((1u128 << niche.value.size(cx).bits()) - 1);
let value = truncate(value, niche.value.size(cx));

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 28, 2019

Member

Oh wow I don't know what I thought when I wrote that code, maybe I just forgot the semantics of <<, even when it doesn't panic - at least it's not UB!

Nevermind, it wasn't me, see #55701 (comment).
Also, the assert_eq! I suggested in da7b6b4, during #54004, was removed, which means debuggers may be getting bad values now (if we started allowing u128 enum discriminants).

@@ -1366,7 +1367,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> {
let value = (i.as_u32() as u128)
.wrapping_sub(niche_variants.start().as_u32() as u128)
.wrapping_add(niche_start);
let value = value & ((1u128 << niche.value.size(cx).bits()) - 1);
let value = truncate(value, niche.value.size(cx));

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 28, 2019

Member

Since you're touching this code, can you add the assert_eq!(value as u64 as u128, value); I'm suggesting in #55701 (comment)?

@eddyb

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@bors r-

@Centril

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

(let's do the other changes in a follow up... this is included in a pending rollup atm... don't force push!)

@eddyb

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Okay, @Centril asked me not to break his rollups so I opened #59509 instead.

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

📌 Commit 8f3e862 has been approved by oli-obk

@bors bors merged commit 8f3e862 into rust-lang:master Mar 29, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@hellow554 hellow554 deleted the hellow554:nonzero_parse branch May 7, 2019

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 31, 2019

rust: Update to 1.35.0.
Version 1.35.0 (2019-05-23)
==========================

Language
--------
- [`FnOnce`, `FnMut`, and the `Fn` traits are now implemented for `Box<FnOnce>`,
  `Box<FnMut>`, and `Box<Fn>` respectively.][59500]
- [You can now coerce closures into unsafe function pointers.][59580] e.g.
  ```rust
  unsafe fn call_unsafe(func: unsafe fn()) {
      func()
  }

  pub fn main() {
      unsafe { call_unsafe(|| {}); }
  }
  ```


Compiler
--------
- [Added the `armv6-unknown-freebsd-gnueabihf` and
  `armv7-unknown-freebsd-gnueabihf` targets.][58080]
- [Added the `wasm32-unknown-wasi` target.][59464]


Libraries
---------
- [`Thread` will now show its ID in `Debug` output.][59460]
- [`StdinLock`, `StdoutLock`, and `StderrLock` now implement `AsRawFd`.][59512]
- [`alloc::System` now implements `Default`.][59451]
- [Expanded `Debug` output (`{:#?}`) for structs now has a trailing comma on the
  last field.][59076]
- [`char::{ToLowercase, ToUppercase}` now
  implement `ExactSizeIterator`.][58778]
- [All `NonZero` numeric types now implement `FromStr`.][58717]
- [Removed the `Read` trait bounds
  on the `BufReader::{get_ref, get_mut, into_inner}` methods.][58423]
- [You can now call the `dbg!` macro without any parameters to print the file
  and line where it is called.][57847]
- [In place ASCII case conversions are now up to 4× faster.][59283]
  e.g. `str::make_ascii_lowercase`
- [`hash_map::{OccupiedEntry, VacantEntry}` now implement `Sync`
  and `Send`.][58369]

Stabilized APIs
---------------
- [`f32::copysign`]
- [`f64::copysign`]
- [`RefCell::replace_with`]
- [`RefCell::map_split`]
- [`ptr::hash`]
- [`Range::contains`]
- [`RangeFrom::contains`]
- [`RangeTo::contains`]
- [`RangeInclusive::contains`]
- [`RangeToInclusive::contains`]
- [`Option::copied`]

Cargo
-----
- [You can now set `cargo:rustc-cdylib-link-arg` at build time to pass custom
  linker arguments when building a `cdylib`.][cargo/6298] Its usage is highly
  platform specific.

Misc
----
- [The Rust toolchain is now available natively for musl based distros.][58575]

[59460]: rust-lang/rust#59460
[59464]: rust-lang/rust#59464
[59500]: rust-lang/rust#59500
[59512]: rust-lang/rust#59512
[59580]: rust-lang/rust#59580
[59283]: rust-lang/rust#59283
[59451]: rust-lang/rust#59451
[59076]: rust-lang/rust#59076
[58778]: rust-lang/rust#58778
[58717]: rust-lang/rust#58717
[58369]: rust-lang/rust#58369
[58423]: rust-lang/rust#58423
[58080]: rust-lang/rust#58080
[57847]: rust-lang/rust#57847
[58575]: rust-lang/rust#58575
[cargo/6298]: rust-lang/cargo#6298
[`f32::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.copysign
[`f64::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.copysign
[`RefCell::replace_with`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.replace_with
[`RefCell::map_split`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.map_split
[`ptr::hash`]: https://doc.rust-lang.org/stable/std/ptr/fn.hash.html
[`Range::contains`]: https://doc.rust-lang.org/std/ops/struct.Range.html#method.contains
[`RangeFrom::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeFrom.html#method.contains
[`RangeTo::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeTo.html#method.contains
[`RangeInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeInclusive.html#method.contains
[`RangeToInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeToInclusive.html#method.contains
[`Option::copied`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.copied
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.