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 AsPrimitive trait for generic casting with as #17

Merged
merged 4 commits into from
Jan 18, 2018
Merged

Add AsPrimitive trait for generic casting with as #17

merged 4 commits into from
Jan 18, 2018

Conversation

Enet4
Copy link
Contributor

@Enet4 Enet4 commented Dec 20, 2017

This is my personal attempt at #7. It is fairly similar to what can be found in asprim, although implemented from scratch. Please let me know of what you think. Could it use more tests? Should I also leave a safety notice that some conversions with as are currently UB (rust-lang/rust#10184)?

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Ah, you've made this a little more powerful than asprim by using a type parameter, so implementations don't have to support all the same types. Neat!

@bluss -- as the author of asprim, I'd like to know what you think of this simplified AsPrimitive.

The tests are fine -- given that Rust doesn't do any implicit conversions here, I think this is one of those "if it compiles, it works" situations. There's no wiggle room for a logic bug here. (Murphy's law -- someone will probably point out how this could have a bug...)

I do think the UB should be mentioned and linked. Another one is rust-lang/rust#15536.

impl_as_primitive!(isize => u8, i8, u16, i16, u32, i32, u64, isize, usize, i64, f32, f64);
impl_as_primitive!(f32 => u8, i8, u16, i16, u32, i32, u64, isize, usize, i64, f32, f64);
impl_as_primitive!(f64 => u8, i8, u16, i16, u32, i32, u64, isize, usize, i64, f32, f64);
impl_as_primitive!(char => char, u8, i8, u16, i16, u32, i32, u64, isize, usize, i64);
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that you include char -- I think we could justify bool to integers as well. Pointers can also use as, but I think it's fair to leave that outside the realm of num.

@Enet4
Copy link
Contributor Author

Enet4 commented Dec 20, 2017

I agree with the conversion from bool to integer types, just punched them in.

I might also add that this implementation has this tiny catch (does asprim have this as well?):

let pi: f64 = 3.1415926535897932384626433832795;
if pi.as_() == 3_u16 {
    println!("All good");
}

The compiler cannot resolve to AsPrimitive<u16> here, because there could be another AsPrimitive<T> where T: PartialEq<u16>. Making it explicit or performing the comparison the other way around works.

let pi: f64 = 3.1415926535897932384626433832795;
if 3_u16 == pi.as_() {
    println!("All good");
}

@Enet4
Copy link
Contributor Author

Enet4 commented Dec 21, 2017

Just a heads-up: I forgot to add a Safety section the other day, so I added it now. What do you think?

I configured the new doctests in the description to be ignored, although they compile fine. I'm not sure whether it's important to compile some UB-inducing code.

@bluss
Copy link
Contributor

bluss commented Dec 21, 2017

Lgtm. Give it a thought -- is this an interface that others can implement, should they, or should they not? Documenting that rule would be good. Most probable rule is that the impl is only "admitted" if there is an corresponding as cast.

@bluss
Copy link
Contributor

bluss commented Dec 21, 2017

As always, the best method for implementation isn't necessarily the best interface for calling. The trait Into incidentally also has this problem. What I think of as the best interface for calling is something that admits foo.as_::<NewType>(). AsPrim has that, incidentally, but it has the other drawbacks of that.

@Enet4
Copy link
Contributor Author

Enet4 commented Dec 22, 2017

Thanks for the feedback, @bluss.

Give it a thought -- is this an interface that others can implement, should they, or should they not? Documenting that rule would be good. Most probable rule is that the impl is only "admitted" if there is an corresponding as cast.

Agreed. It's hard to imagine good use cases for external implementations, though. Would it be for primitive newtypes (say, struct Foo(u32))? I wonder if expressions such as (5.0).as_().into() can be resolved unambiguously.

What I think of as the best interface for calling is something that admits foo.as_::(). AsPrim has that, incidentally, but it has the other drawbacks of that.

Yep, I am aware of this issue. This sounds like a blue-red pill decision between having more possible conversion sources/targets (char and bool) or having a better user-facing API. Whichever is more appealing to the community, I'll make the necessary changes if need be.

On the other hand, given that asprim seems to be well maintained right now, choosing to keep this API here would give users the power to choose either one as they wish (or both, but that would be pointless and just terrible 🙃).

@cuviper
Copy link
Member

cuviper commented Dec 22, 2017

The noisy_float crate comes to mind as an example that might implement this.

@cuviper
Copy link
Member

cuviper commented Jan 13, 2018

I think I'm ready to merge -- are you happy with those final questions of the interface as-is?

@Enet4
Copy link
Contributor Author

Enet4 commented Jan 14, 2018

My apologies, I sort of forgot this PR. In the following hours I could add a statement to the documentation mentioning that external types may implement AsPrimitive when they behave like a primitive numeric type, and the intended conversion does not fail (although it can result in precision loss or truncation). This should enable types such as those found in noisy_float to implement the trait without looking unintuitive.

Update: It's in.

@cuviper
Copy link
Member

cuviper commented Jan 18, 2018

Looks good, thanks!

bors r+

bors bot added a commit that referenced this pull request Jan 18, 2018
17: Add AsPrimitive trait for generic casting with `as` r=cuviper a=Enet4

This is my personal attempt at #7. It is fairly similar to what can be found in `asprim`, although implemented from scratch. Please let me know of what you think. Could it use more tests? Should I also leave a safety notice that some conversions with `as` are currently UB (rust-lang/rust#10184)? 


21: Add checked shifts r=cuviper a=fabianschuiki

Add traits `CheckedShl` and `CheckedShr` that correspond to the standard
library's `checked_shl` and `checked_shr` functions. Implement the trait
on all primitive integer types by default, akin to what the standard
library does.

The stdlib is somewhat inconsistent when it comes to the type of the
shift amount. The `checked_*` functions have a `u32` shift amount, but
the `std::ops::{Shl,Shr}` traits are generic over the shift amount. Also
the stdlib implements these traits for all primitive integer types as
right-hand sides. Our implementation mimics this behaviour.
@bors
Copy link
Contributor

bors bot commented Jan 18, 2018

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants