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 a blanket impl<T: ToPrimitive> ToPrimitive for &T #191

Closed

Conversation

coolreader18
Copy link

Not sure if this is a breaking change or not, but I think this makes sense.

@cuviper
Copy link
Member

cuviper commented Oct 12, 2020

Yes, it is a breaking change, because &T is a fundamental type.

Can you explain why you want to be generic in this way? Maybe it can be done another way, like T: Borrow<U>, U: ToPrimitive to accept either owned or borrowed values.

@coolreader18
Copy link
Author

coolreader18 commented Oct 12, 2020

This is specifically due to a problem I ran into with passing a &BigInt to NumCast::from(), which expects an owned T where T: ToPrimitive. Would it be better to change that to accept a AsRef/Borrow<T>?

@cuviper
Copy link
Member

cuviper commented Oct 14, 2020

Hmm, I doubt we could change NumCast::from without it being a breaking change. I guess we could add a new method like from_ref, but that would need T: Clone + ToPrimitive so it can be defaulted to use from, while all of our impls can call specific to_foo conversions without cloning.

Personally, I avoid NumCast because I don't like how it overlaps with From::from. Directly using ToPrimitive is also better for &BigInt since the methods only require &self.

I'd also suggest the standard TryFrom if possible, stable as of Rust 1.34, but that's not implemented for floating point.

@coolreader18
Copy link
Author

Ah, yeah, TryFrom/Into might be better; I can't use ToPrimitive directly because I'm using a libc type definition. Very specific use case 🙃. I'll close this I guess, but maybe it's something to think if num-traits ever gets a 0.3? Though that's probably unlikely anytime soon.

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

2 participants