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.

Sign up for GitHub

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

Jump to bottom

Make ".".parse::<f32>() and ".".parse::<f64>() return Err #30681

Merged
merged 1 commit into from Jan 5, 2016
Merged

Make ".".parse::<f32>() and ".".parse::<f64>() return Err #30681

merged 1 commit into from Jan 5, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jan 3, 2016

Make ".".parse::<f32>() and ".".parse::<f64>() return Err

This fixes #30344.

This is a [breaking-change], which the libs team have classified as a
bug fix.

@ghost
Copy link
Author

ghost commented Jan 3, 2016

r? @alexcrichton

hanna-kruppe
hanna-kruppe reviewed Jan 3, 2016
View reviewed changes
src/libcore/num/dec2flt/parse.rs
Valid(Decimal::new(integral, b"", 0))
}
None if integral.is_empty() => Invalid, // No digits at all
None => Valid(Decimal::new(integral, b"", 0)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I hastily added this branch in #29050 but I now believe it would be better to check for empty input at the start of the function. Since you're touching this condition anyway, I'd like you to move it there.

@hanna-kruppe
Copy link
Contributor

LGTM modulo nit, thanks!

@ghost ghost changed the title Make ".".parse() return None Make ".".parse() return Err Jan 3, 2016
@bluss
Copy link
Member

bluss commented Jan 3, 2016

Is there a test somewhere that makes sure x. and .y (where x and y are digits) continue to parse as floats?

@ghost
Copy link
Author

ghost commented Jan 3, 2016

x. is tested here and .y is tested here, although I'm not really sure why they're located like that.

@bluss
Copy link
Member

bluss commented Jan 3, 2016

Ok, great!

@ghost
Copy link
Author

ghost commented Jan 4, 2016

@rkruppe nit addressed, I believe

@hanna-kruppe
Copy link
Contributor

Yes that's what I had in mind. Unfortunatly I habe not noticed another thing so I have to ask you to rebase again: The commit message and PR description should explicitly mention float parsing, to be absolutely clear without further context (i.e., when reading bitrust).

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 4, 2016
Make float parsing "." return Err
33f3c52 
This makes both of the following return Err:

    ".".parse::<f32>()
    ".".parse::<f64>()

This is a [breaking-change], which the libs team have classified as a
bug fix.
@ghost ghost changed the title Make ".".parse() return Err Make ".".parse::<f32>() and ".".parse::<f64>() return Err Jan 4, 2016
@ghost
Copy link
Author

ghost commented Jan 4, 2016

How's that @rkruppe?

@hanna-kruppe
Copy link
Contributor

Neat 👍 As far as I am concerned, this PR can be merged, though I can understand if whoever gives the r+ wants to take a closer look for themselves.

Edit: Apparently the fix for barosl/homu#65 still hasn't reached @bors 😢 Well, at least the commit message will be preserved.

@bluss
Copy link
Member

bluss commented Jan 4, 2016

@bors r+

I took the chance to edit the PR message to include the title on the first line. It's what I usually do (until bors is fixed).

Let's go, thanks for the reviewing @rkruppe, it looks correct and I'll rely mostly on you since you know this code so well.

relnote related: Note that this changes how a corner case is parsed as float

@bors
Copy link
Contributor

bors commented Jan 4, 2016

📌 Commit 33f3c52 has been approved by bluss

bors added a commit that referenced this pull request Jan 4, 2016
@bors
Auto merge of #30681 - Toby-S:master, r=bluss
d5e2290 
Make `".".parse::<f32>()` and `".".parse::<f64>()` return Err

This fixes #30344.

This is a [breaking-change], which the libs team have classified as a
bug fix.
@bors
Copy link
Contributor

bors commented Jan 4, 2016

⌛ Testing commit 33f3c52 with merge d5e2290...

@bors
Copy link
Contributor

bors commented Jan 5, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 33f3c52 into rust-lang:master Jan 5, 2016
@brson brson added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jan 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

".".parse() returns Ok(0.0)
6 participants
@hanna-kruppe @bluss @bors @alexcrichton @brson @nikomatsakis