Add FromStr impl for NonZero types by hellow554 · Pull Request #58717 · rust-lang/rust

hellow554 · 2019-02-25T06:13:48Z

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 · 2019-02-25T06:13:50Z

r? @withoutboats

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

Centril · 2019-02-25T06:42:58Z

r? @SimonSapin

hellow554 · 2019-02-26T07:16:05Z

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

SimonSapin · 2019-02-26T16:46:45Z

#[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 · 2019-02-26T16:59:58Z

@rfcbot fcp merge

rfcbot · 2019-02-26T16:59:58Z

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.

src/libcore/num/mod.rs

rfcbot · 2019-02-26T18:40:26Z

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

hellow554 · 2019-02-26T21:11:09Z

@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 · 2019-02-26T21:20:16Z

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.

src/libcore/num/mod.rs

hellow554 · 2019-02-27T06:49:17Z

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

SimonSapin · 2019-02-27T15:22:23Z

@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 · 2019-02-27T16:37:26Z

@SimonSapin that strategy sounds plausible to me!

SimonSapin · 2019-02-27T18:18:29Z

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

rust-highfive · 2019-03-28T09:03:22Z

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)

Fix according to oli-obk (rust-lang#58717 (comment))

oli-obk · 2019-03-28T09:54:05Z

@bors r+

bors · 2019-03-28T09:54:06Z

📌 Commit 8f3e862 has been approved by 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

cuviper · 2019-03-28T15:55:03Z

@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 · 2019-03-28T16:10:38Z

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!

@ghost

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

eddyb · 2019-03-28T22:51:27Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

                                .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));


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).

eddyb · 2019-03-28T23:39:23Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

                                .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));


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 · 2019-03-28T23:39:48Z

@bors r-

Centril · 2019-03-28T23:46:29Z

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

eddyb · 2019-03-28T23:55:01Z

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

@bors r=oli-obk

bors · 2019-03-28T23:55:02Z

📌 Commit 8f3e862 has been approved by oli-obk

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

Add FromStr impl for NonZero types

36bcbc3

rust-highfive assigned withoutboats Feb 25, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2019

rust-highfive assigned SimonSapin and unassigned withoutboats Feb 25, 2019

SimonSapin added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 26, 2019

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 26, 2019

sfackler reviewed Feb 26, 2019

View reviewed changes

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

Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 26, 2019

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 26, 2019

hellow554 commented Feb 27, 2019

View reviewed changes

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 Show resolved Hide resolved

replaced nonzeroparseerror with regular interror

ce30d4e

hellow554 marked this pull request as ready for review February 27, 2019 17:38

SimonSapin added S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2019

rust-highfive assigned oli-obk Mar 28, 2019

Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 28, 2019

fixed shift overflow

8f3e862

Fix according to oli-obk (rust-lang#58717 (comment))

hellow554 force-pushed the nonzero_parse branch from 4d33d57 to 8f3e862 Compare March 28, 2019 09:12

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2019

Centril mentioned this pull request Mar 28, 2019

Rollup of 10 pull requests #59487

Merged

eddyb reviewed Mar 28, 2019

View reviewed changes

eddyb suggested changes Mar 28, 2019

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 28, 2019

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 28, 2019

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

hellow554 deleted the nonzero_parse branch May 7, 2019 06:55

Uh oh!

Conversation

hellow554 commented Feb 25, 2019

Uh oh!

rust-highfive commented Feb 25, 2019

Uh oh!

Centril commented Feb 25, 2019

Uh oh!

hellow554 commented Feb 26, 2019

Uh oh!

SimonSapin commented Feb 26, 2019

Uh oh!

SimonSapin commented Feb 26, 2019

Uh oh!

rfcbot commented Feb 26, 2019 • edited by alexcrichton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rfcbot commented Feb 26, 2019

Uh oh!

hellow554 commented Feb 26, 2019

Uh oh!

SimonSapin commented Feb 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hellow554 commented Feb 27, 2019

Uh oh!

SimonSapin commented Feb 27, 2019

Uh oh!

alexcrichton commented Feb 27, 2019

Uh oh!

SimonSapin commented Feb 27, 2019

Uh oh!

rust-highfive commented Mar 28, 2019

Uh oh!

oli-obk commented Mar 28, 2019

Uh oh!

bors commented Mar 28, 2019

Uh oh!

cuviper commented Mar 28, 2019

Uh oh!

hellow554 commented Mar 28, 2019

Uh oh!

eddyb Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb commented Mar 28, 2019

Uh oh!

Centril commented Mar 28, 2019

Uh oh!

eddyb commented Mar 28, 2019

Uh oh!

bors commented Mar 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

rfcbot commented Feb 26, 2019 •

edited by alexcrichton

Loading