-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 signed num::NonZeroI* types #57475
Conversation
r? @aidanhs (rust_highfive has picked a reviewer for you, use r? to override) |
The new types are stable in this PR, so let’s get team sign-off: @rfcbot fcp merge They don’t really need to be. But they are very similar to the existing unsigned non-zero integer types, so it seems to me that having them as unstable for a period of time is very unlikely to bring new information. Also, making the signed and unsigned type behave differently (beyond signedness) or have different API seems like it would be unfortunate, and the latter are already stable. |
Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. 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. |
The job Click to expand the log.
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 |
I was in favour of not having these before, but there have certainly been enough requests that if these are trivial to add, they might as well just exist. |
I'm in favor of this since it would make |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to have some tests for NonZeroI* types. Check whether they can store negative values without causing undefined behaviour, and so on.
Thank you for this! This is the happiest I've ever been to deprecate my own code. |
The job Click to expand the log.
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 |
I’d appreciate some help. Maybe @alexcrichton? I’m getting errors like ---- num/mod.rs - num::NonZeroI16 (line 33) stdout ----
error[E0412]: cannot find type `NonZeroI16` in module `std::num`
--> num/mod.rs:35:39
|
5 | assert_eq!(size_of::<Option<std::num::NonZeroI16>>(), size_of::<i16>());
| ^^^^^^^^^^ did you mean `NonZeroU16`?
thread 'num/mod.rs - num::NonZeroI16 (line 33)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:321:13 It looks like a newly-added type cannot be found in the doctests of that very type. Later in the output:
… which includes |
The job Click to expand the log.
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 |
Also, |
@SimonSapin hm something looks seriously wrong here! I wonder if this is a preexisting compiler bug you're running into. Can you reproduce the travis failure with |
@alexcrichton Yes, that command on my machine gives the same output as on Travis. |
If I tweak |
Sorry I don't really know what's going on here and what would cause that failure :( It sounds like it may be a build configuration bug or maybe even a rustc bug, but I wouldn't know for sure without investigating. |
@bors: r+ |
📌 Commit 9be4c76 has been approved by |
Add signed num::NonZeroI* types Multiple people have asked for them in rust-lang#49137. Given that the unsigned ones already exist, they are very easy to add and not an additional maintenance burden.
Failed in rollup, #57727 (comment). @bors r- |
The set of impls printed (or at least their order) in the Any idea what’s going on? diff --git a/src/test/ui/try-block/try-block-bad-type.stderr b/src/test/ui/try-block/try-block-bad-type.stderr
index c2f9e9b52b..4e964f4b83 100644
--- a/src/test/ui/try-block/try-block-bad-type.stderr
+++ b/src/test/ui/try-block/try-block-bad-type.stderr
@@ -5,9 +5,9 @@ LL | Err("")?; //~ ERROR the trait bound `i32: std::convert::From<&str>`
| ^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `i32`
|
= help: the following implementations were found:
- <i32 as std::convert::From<core::num::NonZeroI32>>
+ <i32 as std::convert::From<bool>>
<i32 as std::convert::From<i16>>
- <i32 as std::convert::From<i8>>
+ <i32 as std::convert::From<u16>>
<i32 as std::convert::From<u8>>
and 2 others
= note: required by `std::convert::From::from` |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
The diagnostic for this error prints `the following implementations were found` followed by the first N relevant impls, sorted. This commit makes the sort happen before slicing, so that the set of impls being printed is deterministic when the input is not.
I pushed a commit that I hope will fix this by removing some non-determinism. @estebank, could you have a look? Can |
@SimonSapin it is potentially huge, but in practice I would only imagine this being problematic in the case of generated code. But there are two things: 1) this is only relevant for failing code, so the cost only needs to be "human-speed", we're unlikely to hit problems there in any realistic scenario and 2) we might get away with changing I would go ahead with it, maybe adding a debug statement before and after to be able to check after the fact with a pathological case what the time is. |
Good point. I think that means it’s probably not worth worrying about. The other commits have already been reviewed (#57475 (comment)), could you r+ this? |
@bors r+ |
📌 Commit e195ce6 has been approved by |
Sorting can be optimized by using a partial sorting algorithm, changing this from O(N log N) to O(N), but considering this is an error case, it's likely not worth doing. |
Add signed num::NonZeroI* types Multiple people have asked for them in #49137. Given that the unsigned ones already exist, they are very easy to add and not an additional maintenance burden.
☀️ Test successful - checks-travis, status-appveyor |
Multiple people have asked for them in #49137. Given that the unsigned ones already exist, they are very easy to add and not an additional maintenance burden.