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

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

Closed
jturner314 opened this issue Jul 17, 2018 · 2 comments

Comments

@jturner314
Copy link
Contributor

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
Copy link
Member

cuviper commented Jul 17, 2018

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
Copy link
Contributor Author

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!

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

No branches or pull requests

2 participants