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

Integer parsing should accept leading plus #28826

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

arthurprs
Copy link
Contributor

Closes #27580

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@hanna-kruppe
Copy link
Contributor

Damn, I also had a patch for this and was only waiting for the local make check to pass. But mine made the minimal necessary changes and this is probably better, more coherent code, so no harm done.

let mut result = match (c as char).to_digit(radix) {
Some(x) => T::from_u32(x),
let (is_positive, digits) = match src[0] {
b'+' if src.len() == 1 => return Err(PIE { kind: Empty }),
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference would be to not check for empty inputs in the match. Instead I'd slice unconditionally and check if digits.is_empty() afterwards. Seems slightly simpler and shorter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion, I constantly abuse pattern matching...

@alexcrichton
Copy link
Member

Thanks @arthurprs! This implementation looks fine to me, but it's a bit of a significant change to a core function in the standard library, so I'm going to tag this with T-libs to bring up for some broader discussion.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 5, 2015
@alexcrichton
Copy link
Member

cc @rust-lang/libs

@Gankra
Copy link
Contributor

Gankra commented Oct 5, 2015

👍 this seems good

@hanna-kruppe
Copy link
Contributor

Oh my! I just realized that my float rewrite require in #27307 made the analogous change for floats. Compare the output of this code on stable (1.3, branched before the PR was merged) and nightly. I didn't even realize at the time that this was a change of behavior (even though I encountered and worked around the integer version of it), and neither did any of the reviewers — or if they noticed, they didn't mind.

@BurntSushi
Copy link
Member

Are there any downsides to permitting +?

@alexcrichton
Copy link
Member

Nah it seems fine to me, just something to consider and make an explicit decision about! Although given the change @rkruppe mentioned it may already be decided :)

@alexcrichton
Copy link
Member

This libs team discussed this yesterday and the conclusion was to merge. This fits a well accepted precedent from other languages and custom parsing of other formats typically have their own parser anyway so it's unlikely that changes here will affect that form of robust parsers.

Thanks for the PR @arthurprs!

@bors: r+ 123a833

@bors
Copy link
Contributor

bors commented Oct 8, 2015

⌛ Testing commit 123a833 with merge 64c4b51...

bors added a commit that referenced this pull request Oct 8, 2015
@aturon aturon added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 8, 2015
@aturon
Copy link
Member

aturon commented Oct 8, 2015

I'm adding to relnotes, and we should advertise this change widely when it lands since it's a visible behavioral change.

@alexcrichton
Copy link
Member

Oh right, excellent idea!

@bors bors merged commit 123a833 into rust-lang:master Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants