jplatte
commented
5 years ago 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, butSumandProducthave to be imported fromcoresince this is ano_stdcrate. - Relatedly, and this is only a nitpick, but there's no need to refer to their full paths. Maybe refering to them simply as
SumandProductmakes things slightly less clear, but importingcore::iterand referring to them asiter::Sumanditer::Productwould 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 notInt(the reasons for which are layed out in #1). These new impls should stay consistent with the existing operations and wrapUInts 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-I have created apublic) constructors that wrap this logic instead. I don't have a very good idea for a name, butnew_could work.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.
Fixes #10.