-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix println! ICE when parsing percent prefix number #125004
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
Conversation
This comment has been minimized.
This comment has been minimized.
r? estebank though feel free to reassign if you're also not familiar with this code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get of as many unwrap
s as possible in this code. It is always "just" providing a suggestion, so a mistake on the translation is better than an ICE.
Thank you for doing this!
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after the slight change.
Most of the time you have Option<Option<T>>
after a map
, what you really wanted is an and_then
that does Option<K> -> Option<T>
and collapses the None
s.
Thanks! I forgot to use and_then to flatten Option<Option>, the second review change is |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1c90b9f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -0.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.26s -> 667.741s (-0.23%) |
This PR fixes #125002 ICE occurring, for example, with
println!("%100000", 1)
orprintln!("% 100000", 1)
.Test Case/Change Explanation
The return type of
Num::from_str
has been changed toOption<Self>
to handle errors when parsing large integers fails.println!
in the test case covers the change of the firstNum::from_str
usage informat_foreign.rs:426
.println!
in the test case covers the change of the secondNum::from_str
usage in line 460.Num::from_str
usages behave the same as before.The 3rd usage would cause an ICE when
num > u16::MAX
in the previous version, but this commit does not include a fix for the ICE inprintln!("{:100000$}")
. I think we need to emit an error in the compiler and have more discussion in another issue/PR.