Integer parsing should accept leading plus #28826
Conversation
|
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. |
|
Damn, I also had a patch for this and was only waiting for the local |
arthurprs
force-pushed
the
leading_plus
branch
from
ecbfca4
to
7915d8e
Compare
| 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 }), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Great suggestion, I constantly abuse pattern matching...
arthurprs
force-pushed
the
leading_plus
branch
from
7915d8e
to
123a833
Compare
|
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 |
alexcrichton
added
the
T-libs-api
|
cc @rust-lang/libs |
|
👍 this seems good |
|
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. |
|
Are there any downsides to permitting |
|
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 :) |
|
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! |
aturon
added
the
relnotes
|
I'm adding to relnotes, and we should advertise this change widely when it lands since it's a visible behavioral change. |
|
Oh right, excellent idea! |
Closes #27580