improve the TryFrom implementations #43248

llogiq · 2017-07-15T06:39:14Z

This removes the need for a 128 bit storage by making use of the fact that there can be either no over/underflow, either one or both, and each time the target type suffices to hold the limit for comparison. This also means that the implementation will work in targets without 128bit support (unless it's for 128bit types, of course).

The downside is that the code looks a bit more complex.

rust-highfive · 2017-07-15T06:39:25Z

r? @brson

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

llogiq · 2017-07-15T13:05:06Z

r? @BurntSushi since you're already reviewing #43220

oyvindln · 2017-07-15T16:31:22Z

Hm, this might be a bit nicer way to do this than my attempt

BurntSushi · 2017-07-15T16:46:26Z

Thanks @llogiq! This change makes sense to me. My primary concern here is test coverage. What do our tests look like for TryFrom and all these conversions? I don't see any tests in this specific file. Are there tests elsewhere? I'm in particular interested in this because this PR adds target specific impls, and it seems wise to have tests for that.

What do others think? cc @rust-lang/libs

llogiq · 2017-07-15T21:39:52Z

I did not see tests either. I'll write some when I find the time.

I also think I should add #[inline] annotations to all impls, since at least with single bounds, the checks are likely as small as a function call.

llogiq · 2017-07-16T17:27:22Z

@BurntSushi there are tests in src/libcore/tests/num/mod.rs, but they could probably benefit from an update.

alexcrichton · 2017-07-17T15:21:45Z

I agree with @BurntSushi in that it's kind hard to follow what's going on here reading the source code, having an exhaustive test which tests all ints going to all other ints with the same implementation of TryFrom would be nice to have indeed (ideally exericising corner cases and whatnot)

cramertj · 2017-07-17T19:26:31Z

src/libcore/num/mod.rs

-    ($storage:ty, $target:ty, $($source:ty),*) => {$(
+// no possible bounds violation
+macro_rules! try_from_unbounded {
+    ($source:ty, $($target:ty),*) => {$(
        #[unstable(feature = "try_from", issue = "33417")]
        impl TryFrom<$source> for $target {
            type Error = TryFromIntError;


Since this can never fail, shouldn't this be an empty type like enum NeverIntError {}?

Oh-- but I see there are cfg'd impls of this for usize based on the current target's pointer width, so those would have to be kept as TryFromIntError in order to ensure target compatibility.

I think the infallible ones are moving to a blanket impl, per #33417 (comment)

Yep-- I was suggesting enum NeverIntError {} so that this wouldn't be blocked on never_type. Once never_type and the blanket impl land, NeverIntError can be changed to an alias for !.

llogiq · 2017-07-18T03:58:22Z

Ok, I've added the inline annotations and copied over the tests from #43194

kennytm · 2017-07-18T07:59:56Z

src/libcore/num/mod.rs

        #[unstable(feature = "try_from", issue = "33417")]
+        #[inline]
        impl TryFrom<$source> for $target {


[00:01:34] error[E0518]: attribute should be applied to function [00:01:34] --> /checkout/src/libcore/num/mod.rs:2511:9 [00:01:34] | [00:01:34] 2511 | #[inline] [00:01:34] | ^^^^^^^^^ requires a function [00:01:34] ... [00:01:34] 2588 | try_from_unbounded!(u8, u8, u16, u32, u64, u128); [00:01:34] | ------------------------------------------------- in this macro invocation

Yeah shouldn't this be applied to fn try_from?

oyvindln · 2017-07-18T09:31:48Z

src/libcore/num/mod.rs

-                let max = <$signed as FromStrRadixHelper>::max_value() as u128;
-                if u as u128 > max {
+            fn try_from(u: $source) -> Result<$target, TryFromIntError> {
+                if u > (<$target as FromStrRadixHelper>::max_value() as $source) {


The as FromStrRadixHelper isn't actually needed. You can simply do <$target>::max_value() and avoid the extra indirection.

scottmcm · 2017-07-19T06:47:18Z

Looks like a real failure?

Testing libstd stage2 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:00:21]    Compiling rand v0.0.0 (file:///checkout/src/librand)
[01:00:21]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[01:00:21]    Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[01:00:21]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[01:00:27]    Compiling collections v0.0.0 (file:///checkout/src/libcollections)
[01:00:27]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[01:00:33] error[E0277]: the trait bound `u64: core::convert::TryFrom<isize>` is not satisfied
[01:00:33]    --> /checkout/src/libcore/../libcore/tests/num/mod.rs:349:24
[01:00:33]     |
[01:00:33] 349 |             assert_eq!(<$target as TryFrom<$source>>::try_from(max).unwrap(),
[01:00:33]     |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `core::convert::TryFrom<isize>` is not implemented for `u64`
[01:00:33] ...
[01:00:33] 383 |     test_impl_try_from_signed_to_unsigned_upper_ok! { test_try_isizeu64, isize, u64 }
[01:00:33]     |     --------------------------------------------------------------------------------- in this macro invocation
[01:00:33]     |
[01:00:33]     = help: the following implementations were found:
[01:00:33]               <u64 as core::convert::TryFrom<u16>>
[01:00:33]               <u64 as core::convert::TryFrom<i128>>
[01:00:33]               <u64 as core::convert::TryFrom<u64>>
[01:00:33]               <u64 as core::convert::TryFrom<usize>>
[01:00:33]             and 7 others

llogiq · 2017-07-19T19:11:09Z

That's strange... Is the target pointer size not one of 16/32/64 on that build? I note that all other builds are successful.

Perhaps I should add code for this scenario.

oyvindln · 2017-07-19T19:31:05Z

It says x86_64-unknown-linux-gnu so it should be 64-bit. I don't think any of the other builds run any tests, did it work locally?

EDIT: The tests you borrowed from my pr even has a check for non 16/32/64-bit in the "assume_ptr_width" macro that wraps the pointer width specific tests, so that's definitely not the issue.

llogiq · 2017-07-20T04:39:23Z

OK, but then why does the error only appear on that one LLVM 3.7 builder?

scottmcm · 2017-07-20T05:42:42Z

@llogiq Because for PRs, the other jobs are configured to do essentially nothing. You'll see "skipping, not a full build" in them.

kennytm · 2017-07-21T02:25:27Z

src/libcore/num/mod.rs

-#[cfg(target_pointer_width = "64")] try_from_both_bounded!(i128, usize);
+
+cfg_block!{
+    #[cfg(target_pointer_width = "16")] {


Can these be done with inner modules instead?

#[cfg(target_pointer_width = "16")] mod imp { ... } #[cfg(target_pointer_width = "32")] mod imp { ... } pub use imp::*;

You could probably also do the same in the tests.

llogiq · 2017-07-21T02:26:48Z

@kennytm of course, I just reused the macro from the tests.Will wait for travis, though, so I can fix whatever it comes up with.

llogiq · 2017-07-21T20:05:10Z

By, the way, I'd like to leave the macro in the tests for now, as I think submodules will get their own headers in the test output, right?

llogiq · 2017-07-21T22:14:22Z

Huh? That tidy error 1 sure looks spurious.

oyvindln · 2017-07-21T22:30:16Z

[00:01:34] error: expected item after attributes

[00:01:34]     --> /checkout/src/libcore/num/mod.rs:2638:35

[00:01:34]      |

[00:01:34] 2638 | #[cfg(target_pointer_width = "16")] {

[00:01:34]      |                                   ^

[00:01:34] 

[00:01:34] error: aborting due to previous error(s)

[00:01:34] 

[00:01:34] error: Could not compile `core`.

Looks like there is an extra set of braces. I think you also forgot to pub use the module.

llogiq · 2017-07-22T13:09:29Z

Ah, thanks. Will fix once I get to my PC.

kennytm · 2017-07-23T09:31:05Z

src/libcore/num/mod.rs

+try_from_both_bounded!(i64, u32, u16, u8);
+try_from_both_bounded!(i128, u64, u32, u16, u8);
+
+pub use ptr_try_from_impls.*;


more typo

[00:01:56] Compiling unwind v0.0.0 (file:///checkout/src/libunwind) [00:01:56] error: expected one of `::`, `;`, or `as`, found `.` [00:01:56] --> /checkout/src/libcore/num/mod.rs:2630:27 [00:01:56] | [00:01:56] 2630 | pub use ptr_try_from_impls.*; [00:01:56] | ^ expected one of `::`, `;`, or `as` here

nagisa · 2017-07-24T16:53:06Z

@llogiq will you be fixing this? The alternate PR has been ready for quite a while, although it seems to me like having #[cfg] mod is slightly more clean, so I would rather land this.

I’ll wait for this to get into landable state or till I get another triage ping on the alternate PR whichever comes sooner to take a decision here.

nagisa · 2017-07-24T16:56:35Z

Also I feel like @oyvindln deserves some attribution in the commit message for authoring the tests.

llogiq · 2017-07-24T17:20:01Z

@nagisa I wholeheartedly agree. The only reason I didn't merge his PR directly is I suck at git. 🙁

oyvindln · 2017-07-24T17:22:26Z

@nagisa @llogiq Thanks!

llogiq · 2017-07-24T19:25:24Z

This appears to work. I'll try to squash & add proper attribution in the commit message.

@oyvindln

This removes the need for a 128 bit storage by making use of the fact that there can be either no over/underflow, either one or both, and each time the target type suffices to hold the limit for comparison. The downside is that the code looks a bit more complex. This test code included in this commit is from @oyvindln 's PR. They also greatly helped fixing a number of errors I made along the way. Thanks a lot!

llogiq · 2017-07-24T20:41:58Z

Done!

nagisa · 2017-07-24T20:47:02Z

@bors r+

bors · 2017-07-24T20:47:03Z

📌 Commit 72ef15e has been approved by nagisa

ollie27 · 2017-07-24T20:07:56Z

src/libcore/num/mod.rs

+try_from_both_bounded!(i128, u64, u32, u16, u8);
+
+#[unstable(feature = "try_from", issue = "33417")]
+pub use self::ptr_try_from_impls::*;


What is this for? You can't (and don't need to) reexport impls.

Really? Because the pub makes them visible where the mods are not. I deem them implementation detail, so I chose to keep them private.

Impls are always visible, even if they're in private modules.

ollie27 · 2017-07-24T20:30:38Z

src/libcore/tests/num/mod.rs

+test_impl_try_from_always_ok! { test_try_u64u128, u64, u128 }
+test_impl_try_from_always_ok! { test_try_u64i128, u64, i128 }
+
+test_impl_try_from_always_ok! { test_try_u128, u128, u128 }


For consistency with the other tests this should be test_try_u128u128.

ollie27 · 2017-07-24T20:41:48Z

src/libcore/num/mod.rs

+    try_from_both_bounded!(isize, u8);
+    try_from_lower_bounded!(isize, usize, u16, u32, u64, u128);
+    try_from_both_bounded!(isize, i8);
+    try_from_unbounded!(isize, i16, i32, i64, i128);


This looks like it's missing isize to isize. It might be clearer to put the following outside these target specific modules:

try_from_unbounded!(usize, usize); try_from_upper_bounded!(usize, isize); try_from_lower_bounded!(isize, usize); try_from_unbounded!(isize, isize);

ollie27 · 2017-07-24T20:44:33Z

src/libcore/tests/num/mod.rs

+    cfg_block!(
+        #[cfg(target_pointer_width = "16")] {
+            test_impl_try_from_always_ok! { test_try_u16usize, u16, usize }
+            test_impl_try_from_always_ok! { test_try_i16isize, i16, isize }


these should be:

test_impl_try_from_always_ok! { test_try_usizeu16, usize, u16 } test_impl_try_from_always_ok! { test_try_isizei16, isize, i16 }

also missing:

test_impl_try_from_always_ok! { test_try_usizeu32, usize, u32 } test_impl_try_from_always_ok! { test_try_usizei32, usize, i32 } test_impl_try_from_always_ok! { test_try_usizei64, usize, i64 } test_impl_try_from_always_ok! { test_try_isizei32, isize, i32 }

ollie27 · 2017-07-24T20:52:37Z

src/libcore/tests/num/mod.rs

+test_impl_try_from_always_ok! { test_try_i64i128, i64, i128 }
+
+test_impl_try_from_always_ok! { test_try_i128i128, i128, i128 }
+


missing:

test_impl_try_from_always_ok! { test_try_usizeusize, usize, usize } test_impl_try_from_always_ok! { test_try_isizeisize, isize, isize }

ollie27 · 2017-07-24T21:03:51Z

src/libcore/tests/num/mod.rs

+test_impl_try_from_always_ok! { test_try_i128i128, i128, i128 }
+
+assume_usize_width! {
+    test_impl_try_from_always_ok! { test_try_u8usize, u8, usize }


missing:

test_impl_try_from_always_ok! { test_try_u8isize, u8, isize }

ollie27 · 2017-07-24T21:19:45Z

src/libcore/tests/num/mod.rs

+            test_impl_try_from_always_ok! { test_try_usizeu32, usize, u32 }
+            test_impl_try_from_always_ok! { test_try_isizei32, isize, i32 }
+            test_impl_try_from_always_ok! { test_try_u32usize, u32, usize }
+            test_impl_try_from_always_ok! { test_try_i32isize, i32, isize }


missing:

test_impl_try_from_always_ok! { test_try_u16isize, u16, isize } test_impl_try_from_always_ok! { test_try_usizei64, usize, i64 }

ollie27 · 2017-07-24T21:19:56Z

src/libcore/tests/num/mod.rs

+            test_impl_try_from_always_ok! { test_try_u32usize, u32, usize }
+            test_impl_try_from_always_ok! { test_try_i32isize, i32, isize }
+            test_impl_try_from_always_ok! { test_try_u64usize, u64, usize }
+            test_impl_try_from_always_ok! { test_try_i64isize, i64, isize }


missing:

test_impl_try_from_always_ok! { test_try_u16isize, u16, isize } test_impl_try_from_always_ok! { test_try_u32isize, u32, isize }

ollie27 · 2017-07-24T21:27:30Z

src/libcore/tests/num/mod.rs

+    cfg_block!(
+        #[cfg(target_pointer_width = "16")] {
+            test_impl_try_from_unsigned_to_signed_upper_err! { test_try_u32isize, u32, isize }
+            test_impl_try_from_unsigned_to_signed_upper_err! { test_try_u32isize, u64, isize }


this one should be:

test_impl_try_from_unsigned_to_signed_upper_err! { test_try_u16isize, u16, isize }

bors · 2017-07-25T00:48:22Z

⌛ Testing commit 72ef15e with merge 7c46c6c...

improve the TryFrom implementations This removes the need for a 128 bit storage by making use of the fact that there can be either no over/underflow, either one or both, and each time the target type suffices to hold the limit for comparison. This also means that the implementation will work in targets without 128bit support (unless it's for 128bit types, of course). The downside is that the code looks a bit more complex.

bors · 2017-07-25T03:22:26Z

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

llogiq · 2017-07-25T04:20:27Z

@ollie27 thanks for noticing the missing tests! Do you want to do a followup PR adding them?

ollie27 · 2017-07-25T04:39:24Z

Sure, I'll add them in a new PR.

ollie27 · 2017-07-25T05:29:40Z

Done: #43471

cramertj · 2017-07-25T06:33:46Z

I'm confused why this was merged-- don't the infallible TryFrom implementations here conflict with the future blanket impl for From?

kennytm · 2017-07-25T06:53:58Z

@cramertj this can be reverted once that NumCast RFC is implemented (which was scheduled to be submitted mid-August).

scottmcm · 2017-07-25T06:58:56Z

@cramertj the infallible impls were already there (https://doc.rust-lang.org/std/convert/trait.TryFrom.html), so AFAIK they'll be removed only when that blanket impl is added.

@nagisa

Add missing impl and tests for int to int TryFrom impls These were missing from rust-lang#43248. r? @nagisa

rust-highfive assigned brson Jul 15, 2017

rust-highfive assigned BurntSushi and unassigned brson Jul 15, 2017

oyvindln mentioned this pull request Jul 15, 2017

Add inline annotations and avoid 128-bit where it's not needed in TryFrom + tests for xsize/x128 #43194

Closed

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2017

BurntSushi 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2017

cramertj reviewed Jul 17, 2017

View reviewed changes

kennytm reviewed Jul 18, 2017

View reviewed changes

oyvindln reviewed Jul 18, 2017

View reviewed changes

kennytm reviewed Jul 21, 2017

View reviewed changes

llogiq force-pushed the num-try-from branch from 8a0314e to 048a27c Compare July 23, 2017 00:25

kennytm reviewed Jul 23, 2017

View reviewed changes

SimonSapin mentioned this pull request Jul 24, 2017

Redesign the std::iter::Step trait, tweak related iterator impls for ranges #43127

Closed

llogiq force-pushed the num-try-from branch from 2ab762c to 72ef15e Compare July 24, 2017 20:41

ollie27 suggested changes Jul 24, 2017

View reviewed changes

bors merged commit 72ef15e into rust-lang:master Jul 25, 2017

bors mentioned this pull request Jul 25, 2017

Add TryFrom implementation for bool, f32 and f64 #43220

Closed

llogiq deleted the num-try-from branch July 25, 2017 04:18

ollie27 mentioned this pull request Jul 25, 2017

Add missing impl and tests for int to int TryFrom impls #43471

Merged

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 26, 2017

Rollup merge of rust-lang#43471 - ollie27:try_from_ints, r=nagisa

ed09d6d

Add missing impl and tests for int to int TryFrom impls These were missing from rust-lang#43248. r? @nagisa

oyvindln mentioned this pull request Jul 31, 2017

Collect<Vec<u16>> from range doesn't optimize well. #43124

Closed

alexcrichton mentioned this pull request Aug 12, 2017

Slower binaries since some days #42935

Closed

scottmcm mentioned this pull request Apr 2, 2019

Implement useful steps_between for all integers #59444

Merged

		test_impl_try_from_always_ok! { test_try_i64i128, i64, i128 }

		test_impl_try_from_always_ok! { test_try_i128i128, i128, i128 }

improve the TryFrom implementations #43248

improve the TryFrom implementations #43248

Conversation

llogiq commented Jul 15, 2017 • edited Loading

rust-highfive commented Jul 15, 2017

llogiq commented Jul 15, 2017

oyvindln commented Jul 15, 2017 • edited Loading

BurntSushi commented Jul 15, 2017

llogiq commented Jul 15, 2017

llogiq commented Jul 16, 2017

alexcrichton commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Jul 19, 2017

llogiq commented Jul 19, 2017

oyvindln commented Jul 19, 2017 • edited Loading

llogiq commented Jul 20, 2017

scottmcm commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Jul 21, 2017

llogiq commented Jul 21, 2017

llogiq commented Jul 21, 2017

oyvindln commented Jul 21, 2017

llogiq commented Jul 22, 2017

Choose a reason for hiding this comment

nagisa commented Jul 24, 2017

nagisa commented Jul 24, 2017

llogiq commented Jul 24, 2017

oyvindln commented Jul 24, 2017

llogiq commented Jul 24, 2017

llogiq commented Jul 24, 2017

nagisa commented Jul 24, 2017

bors commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 25, 2017

bors commented Jul 25, 2017

llogiq commented Jul 25, 2017

ollie27 commented Jul 25, 2017

ollie27 commented Jul 25, 2017

cramertj commented Jul 25, 2017 • edited Loading

kennytm commented Jul 25, 2017

scottmcm commented Jul 25, 2017

llogiq commented Jul 15, 2017 •

edited

Loading

oyvindln commented Jul 15, 2017 •

edited

Loading

oyvindln commented Jul 19, 2017 •

edited

Loading

cramertj commented Jul 25, 2017 •

edited

Loading