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
Merged

Conversation

hellow554
Copy link
Contributor

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
Copy link
Collaborator

r? @withoutboats

(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 Feb 25, 2019
@Centril
Copy link
Contributor

Centril commented Feb 25, 2019

r? @SimonSapin

@hellow554
Copy link
Contributor Author

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

@SimonSapin
Copy link
Contributor

#[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-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 26, 2019
@SimonSapin
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

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

@rfcbot 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
src/libcore/num/mod.rs Outdated Show resolved Hide resolved
@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 26, 2019
@rfcbot 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
@rfcbot
Copy link

rfcbot commented Feb 26, 2019

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

@hellow554
Copy link
Contributor Author

@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
Copy link
Contributor

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 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
@hellow554
Copy link
Contributor Author

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

@SimonSapin
Copy link
Contributor

@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
Copy link
Member

@SimonSapin that strategy sounds plausible to me!

@hellow554 hellow554 marked this pull request as ready for review February 27, 2019 17:38
@SimonSapin SimonSapin added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2019
@SimonSapin
Copy link
Contributor

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

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

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)

@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2019

📌 Commit 8f3e862 has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 28, 2019
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
Copy link
Member

cuviper 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
Copy link
Contributor Author

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
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));
Copy link
Member

Choose a reason for hiding this comment

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

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));
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

eddyb commented Mar 28, 2019

@bors r-

@bors 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
@Centril
Copy link
Contributor

Centril 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
Copy link
Member

eddyb commented Mar 28, 2019

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

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Mar 28, 2019

📌 Commit 8f3e862 has been approved by oli-obk

@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 28, 2019
@bors bors merged commit 8f3e862 into rust-lang:master Mar 29, 2019
@hellow554 hellow554 deleted the nonzero_parse branch May 7, 2019 06:55
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 31, 2019
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
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.