-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
integers shouldn't implement num::Round #4789
Comments
Related: #4788 |
But I definitely feel that the current implementation is over-engineered, and rounding functions should not be impled on integer types. |
Here is the current implementation:
|
To follow up, I've moved the rounding functions into a separate trait. I think that's a better design actually. I haven't replicated |
I feel a bit uneasy about a Haskell declares I think having a Fractional trait would not be a bad idea. |
Ooh, that's very handy. Thank you! |
@kud1ing: how about this?
|
@pcwalton ping^ |
Sounds good to me… I don't have much of an opinion here, really. |
As the one who is responsible for this trait, I fully agree that implementing it on integers is not a good solution, but together with the generic NaN/inf functions (#4788) it was necessary for the str conversion functions in num to work on all numbers. (Which in itself might be a bad idea, I don't know.) However, I think that at least having the Roundmode is worthwhile (and, again, necessary for the conversions functions to work), as it really just allows to treat rounding in a symmetric way in regards to the sign.
It's really just about considering the sign. |
Also, for the Bounded trait, maybe it might make sense to have a more general 'Limits' trait where upperLimit and lowerLimit would be defined as one of:
|
@Kimundi While I don't think that having a RoundingMode argument really makes sense when compared to just having multiple methods, the most important problem here is that you've missed the IEEE754 default rounding mode, round-to-even. (Maybe something like this? https://gist.github.com/JensNockert/4719287/raw/65c32ad7eb64e67b55b107e5afb4dfe6e6f6dac6/rounding.rs) When it comes to a bounded trait, we could just check http://www.cplusplus.com/reference/limits/numeric_limits/ for example, I assume they have been thinking and arguing about this for a very long while and it seems quite sane. |
@bjz: Nice. I am not sure, whether separate Neg, Add, Sub, Mul are necessary. Also not sure about Real. Maybe we should create a Wiki page for this, explaining what traits we need and what for. |
ping @erickt (added Zero and One) |
@kud1ing: I started doing some thinking on this yesterday, just a sketch, not thought through https://gist.github.com/JensNockert/4719287 and there are loads of refactoring opportunities. But separate Neg, Add, Sub, Mul are necessary, not for numbers, but for other types. String-like types could have an add and mul, but no neg/sub for example. Arrays could have Add, Sub and Mul without Neg. Sets could have Add, Neg and Sub without Mul. The list of silly usecases that we might never think of is endless, pairing the operators now wouldn't really make sense. I proposed above instead traits that include them in pairs for types that implement them in a reasonably mathematical way, called Additive / Multiplicative, to reduce typing. |
@JensNockert; nice |
Looks like a good improvement to my effort @JensNockert. :) Now we need to figure out how to fit in FromStr/ToStr. |
A word of warning, we'll need #4183 to be fixed before we can inherit the operator traits. |
I created a new (meta?) bug #4819 for the numeric traits so it can be found easier, since we kind of expanded the scope of this one by huge amounts. |
This can be closed now. |
Fixed in bd93a36 |
After discussions on IRC and rust-lang#4789, we have decided to revert this change. This is due to the traits expressing different ideas and because hyperbolic functions are not trivially implementable from exponential functions for floating-point types.
They don't have a sensible implementation.
The text was updated successfully, but these errors were encountered: