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

Implement std::iter::Sum and std::iter::Product #11

Closed
wants to merge 3 commits into from

Conversation

Kroisse
Copy link

@Kroisse Kroisse commented Jul 11, 2019

Fixes #10.

@jplatte
Copy link
Member

jplatte commented Jul 15, 2019

Hi! Thanks for your PR :)

Overall, the code looks good to me, but there are a few things that could be improved. Don't be overwhelmed by the amount of text below, it's all small things and I'm happy to answer further questions.

  • The issue mentions std::iter::Sum, but Sum and Product have to be imported from core since this is a no_std crate.
  • Relatedly, and this is only a nitpick, but there's no need to refer to their full paths. Maybe refering to them simply as Sum and Product makes things slightly less clear, but importing core::iter and referring to them as iter::Sum and iter::Product would be more consistent with the rest of the code while saving some characters for people reading the code.
  • I haven't found documentation about it, but I would assume the standard library implementations of these traits to overflow / underflow in release mode for performance reasons just like other arithmetic operations. I've done the same for arithmetic operations for UInts but not Int (the reasons for which are layed out in How to implement signed over- / underflow for Int? #1). These new impls should stay consistent with the existing operations and wrap UInts in release mode. See https://github.com/jplatte/js_int/blob/75868bcc7722a934ba61b24782e88743a0a4a501/src/lib.rs#L894-L899
  • The range assertions are duplicated a few times. Implementing the change I suggested in the previous point will only increase the amount of duplicated lines. I think it would make sense to use (non-public) constructors that wrap this logic instead. I don't have a very good idea for a name, but new_ could work. I have created a new_ method as I suggested in the crossed-out sentence. If you rebase your PR you can use them.
  • Considering the code being tested is absolutely trivial, even more so if the suggestion from my previous point is implemented I think it's not necessary to add tests for every impl. I'd rather see edge cases tested, although I also could live without any tests for these impls.

@jplatte
Copy link
Member

jplatte commented Aug 4, 2019

Done in ea03c1b

@jplatte jplatte closed this Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Implement std::iter::Sum for Int, UInt
2 participants