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 "infinity" and ignore case when parsing floats. #48886

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

This fixes #48881.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2018
@pietroalbini
Copy link
Member

Thanks for this PR! Unfortunately, highfive failed to assign a reviewer to this PR. Picking someone randomly from the libs team. @dtolnay?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

The code looks good but what do you see as the use cases of this? What are some examples where an application would not have been able to use f64::from_str before but would want to after this change?

@petrochenkov
Copy link
Contributor

ping @rkruppe

@clarfonthey
Copy link
Contributor Author

@dtolnay there are a few examples in the original issue; I think that once inf, INF, NaN, NAN, and nan are allowed, it just makes more sense to simplify the code and allow any case. The usage of "infinity" was just an extra I thought of though, and can be removed if it seems unnecessary.

@clarfonthey clarfonthey force-pushed the float_parse branch 2 times, most recently from 4a21eab to 157b035 Compare March 12, 2018 01:58
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I just feel that "can unambiguously be interpreted as an f64" is not the set of inputs that f64::from_str would ideally accept. We define what the function accepts, and if someone wants to call the function they are expected to pass input the way we defined it. For that purpose I see the current set of "inf" and "NaN" as being sufficient. If someone is writing a YAML library for example and YAML's infinity is spelled ".inf", there should be no expectation that f64::from_str is the tool they need to use.

@clarfonthey
Copy link
Contributor Author

That is understandable! If you think that this wouldn't be worth having I can close this PR.

@dtolnay
Copy link
Member

dtolnay commented Mar 13, 2018

I appreciate the PR but let's keep this the way it is. Thanks!

@JohnBSmith
Copy link

It seems understandable to me too, I have found more than one reason that speaks against it. First of all, this would have been a subtle breaking change, as the return value's specification including its error cases belongs to the interface. Then, making a small and clear interface more complex is controversial and should not be taken lightly. And if the exact syntax definition differs, a validating parser needs to handle the cases that don't match [sign] ('.' | digit) by itself anyway.

@clarfonthey clarfonthey deleted the float_parse branch April 15, 2018 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making f64::from_str case insensitive
5 participants