rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
731 stars 135 forks source link

Allow classification of Float and FloatCore values without taking ownership #76

Closed jturner314 closed 6 years ago

jturner314 commented 6 years ago

The .is_nan(), .is_infinite(), .is_finite(), .is_normal(), .classify(), .is_sign_positive(), and .is_sign_negative() methods of the Float and FloatCore traits take ownership of self. This is problematic when writing generic code for container types which contain T: Float because in many cases, only &T is available and it's not guaranteed that T implements Copy or Clone. Taking ownership is also problematic for arbitrary precision floating point types which are expensive to clone. I can't think of any reasonable case where taking ownership would be necessary to implement these methods.

Taking &self instead of self should not incur any performance penalty for f32 and f64 as long as the call is inlined; the #[inline] attribute is already included on all of those methods. As a demonstration, call() and call_with_ref() generate the same assembly after optimization in this example (is_nan() and is_nan_ref() are inlined):

fn is_nan(num: f32) -> bool {
    num.is_nan()
}

fn is_nan_ref(num: &f32) -> bool {
    num.is_nan()
}

pub fn call(num: f32) -> bool {
    is_nan(num)
}

pub fn call_with_ref(num: f32) -> bool {
    is_nan_ref(&num)
}

I propose doing either one of the of the following for .is_nan(), .is_infinite(), .is_finite(), .is_normal(), .classify(), .is_sign_positive(), and .is_sign_negative():

  1. Change the existing methods to take &self instead of self.

  2. Add methods (e.g. .is_nan_ref() or .is_nan_borrowed()) which take &self instead of self.

cuviper commented 6 years ago

This is problematic when writing generic code for container types which contain T: Float because in many cases, only &T is available and it's not guaranteed that T implements Copy or Clone.

Float and FloatCore both do currently require Copy, and the compiler should let you use that implicitly for any T: Float or T: FloatCore.

It's a separate question whether they should require Copy. It would probably be nicer to limit that to a PrimFloat analogue to PrimInt. I agree that all those methods should use &self if we didn't have a Copy bound. This would of course be a breaking change -- #47.

jturner314 commented 6 years ago

Float and FloatCore both do currently require Copy

Ah, yes, you're right; I missed that. I still haven't gotten used to rustdoc hiding the trait declaration by default. I'm sorry for the trouble.

t's a separate question whether they should require Copy.

I lean towards "no", but that's a separate issue.

Thanks for the quick response!