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

Fix for issue #7588, "Overflow handling of from_str methods is broken" #9010

Closed
wants to merge 1 commit into from
Closed

Conversation

aaronlaursen
Copy link
Contributor

Here's a fix for issue #7588, "Overflow handling of from_str methods is broken".

The integer overflow issues are taken care of by checking to see if the multiply-by-radix-and-add-next-digit process is reversible. If it overflowed, then some information is lost and the process is irreversible, in which case, None is returned.

Floats now consistently return Some(Inf) of Some(-Inf) on overflow thanks to a call to NumStrConv::inf() and NumStrConv::neg_inf() respectively when the overflow is detected (which yields a value of None in the case of ints and uints anyway).

This is my first contribution to Rust, and my first time using the language in general, so any and all feedback is appreciated.

@alexcrichton
Copy link
Member

Thanks for the contribution! Before we merge this though, I think this probably needs two things:

  1. Some unit tests would be useful in preventing this to ever regress again.
  2. Could you squash all the commits into just one?

@aaronlaursen
Copy link
Contributor Author

sounds good-
I'll look into the proper ways to implement these

On Thu, Sep 5, 2013 at 9:25 PM, Alex Crichton notifications@github.comwrote:

Thanks for the contribution! Before we merge this though, I think this
probably needs two things:

  1. Some unit tests would be useful in preventing this to ever regress
    again.
  2. Could you squash all the commits into just one?


Reply to this email directly or view it on GitHubhttps://github.com//pull/9010#issuecomment-23915232
.

@aaronlaursen
Copy link
Contributor Author

Unit tests are implemented, and the commits are squashed.

Let me know if you think anything else needs to be done.

On Thu, Sep 5, 2013 at 10:51 PM, Aaron Laursen alaursen@macalester.eduwrote:

sounds good-
I'll look into the proper ways to implement these

On Thu, Sep 5, 2013 at 9:25 PM, Alex Crichton notifications@github.comwrote:

Thanks for the contribution! Before we merge this though, I think this
probably needs two things:

  1. Some unit tests would be useful in preventing this to ever regress
    again.
  2. Could you squash all the commits into just one?


Reply to this email directly or view it on GitHubhttps://github.com//pull/9010#issuecomment-23915232
.

bors added a commit that referenced this pull request Sep 7, 2013
Here's a fix for issue #7588, "Overflow handling of from_str methods is broken". 

The integer overflow issues are taken care of by checking to see if the multiply-by-radix-and-add-next-digit process is reversible. If it overflowed, then some information is lost and the process is irreversible, in which case, None is returned. 

Floats now consistently return Some(Inf) of Some(-Inf) on overflow thanks to a call to NumStrConv::inf() and NumStrConv::neg_inf() respectively when the overflow is detected (which yields a value of None in the case of ints and uints anyway). 

This is my first contribution to Rust, and my first time using the language in general, so any and all feedback is appreciated.
@bors bors closed this Sep 7, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2023
initial clippy::ref_pattern implementation

This implements a new lint that discourages the use of the `ref` keyword as outlined in rust-lang#9010. I think there are still some things to improve about this lint, but I need some feedback before I can implement those.

- [x] ~~Maybe it's desirable that a quick fix is listed too, instead of a generic `avoid using the ref keyword` lint.~~
- [x] `let ref x = y` already has a lint (`clippy::toplevel_ref_arg`). This implementation will report this too, so you get two lints for the same issue, which is not great. I don't really know a way around this though.
- [X] The dogfood test is currently failing locally, though I ran `cargo clippy -- -D clippy::all -D clippy::pedantic` and got no output, so I'm not sure why this is.

Any help with these would be greatly appreciated.

fixes rust-lang#9010
changelog: [`ref_pattern`]: newly added lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants