search

Robbepop / apint

Arbitrary precision integers library.
Other
26 stars 4 forks source link

Utilities and Arithmetic Rewrite #30

Closed AaronKutch closed 6 years ago

AaronKutch commented 6 years ago

My second big pull request is finally complete. In the end, I realized that this commit has essentially rewritten arithmetic.rs and many utilities, and "multiple commits" in this case would entail each commit being the breaking rewrite of one file. We might as well keep this as one big commit. Trust me when I say that it is a better idea than what my commit history would have looked like.

This includes the renaming of hi_lo functions and the zip functions, implementing division and remainder (NOTE: the functions pass both of my special purpose fuzz testers, but there is still a lot of work for me to do in my division function. The only difference now is that we can much more easily work in parallel), fixing up certian docs, deleting ll.rs (I might make a new one later), rewriting the testing functions, and a few other things.

When you compile this you will get two macro use warnings but those can be fixed after your refactor to Rust 2018, which has slightly different macro imports and stuff.

AaronKutch commented 6 years ago

Edit: I have gotten much better at Git. I should have looked at more tutorials because you can simply click inside GitHub desktop to select or unselect lines you want to commit

AaronKutch commented 6 years ago

Also, when you cargo doc this you can also add --document-private-items now

Robbepop commented 6 years ago

Again thank you so much for all the excellent work you have put into this library! :) I really want to merge this. See my PR review above. I think I should mention you as official co-author of this library.

AaronKutch commented 6 years ago

Do I just click resolve conversation if I did a quick fix for it? Also, I forgot about how to change the commits in the pull request without closing it.

Robbepop commented 6 years ago

I am not sure either but the safest option is to just append commits as fixes. It doesn't matter to me if you have some additional well-structured commits behind this big one.

AaronKutch commented 6 years ago

It appears that the pull request automatically updates when I force the commit

AaronKutch commented 6 years ago

There were no perf regressions from removing the inline(always). The two overflowing functions and the from_vec_u64 I will fix or rewrite in the future, I have made them pub(crate) for now.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+4.6%) to 75.044% when pulling d737809525c7c289615add8b1252c53548ae7699 on AaronKutch:master into 55e7ab527d4a78fd8b0e5722bf0bab1379ea22aa on Robbepop:master.

AaronKutch commented 6 years ago

What are the automatic checks (like travis-ci) testing for?

Robbepop commented 6 years ago

There were no perf regressions from removing the inline(always). The two overflowing functions and the from_vec_u64 I will fix or rewrite in the future, I have made them pub(crate) for now.

Thank you for that - we will decide upon their stabilization as public API simply in a later discussion. ;)

Robbepop commented 6 years ago

It appears that the pull request automatically updates when I force the commit

That is at least how I also always update my PR commit history.

AaronKutch commented 6 years ago

Are there any other problems to fix before accepting the PR?

AaronKutch commented 6 years ago

I also haven't seen you comment in the refactoring issue. Have you seen it or did the Github notification go in the email spam folder like it does to me sometimes?

Robbepop commented 6 years ago

I am so sorry. I will answer you tomorrow when I find time for it. Hope that is okay. I think at least to me it looks like there are no major problems that should prevent this from being merged. ;)

Robbepop commented 6 years ago

I think this is ready for merging. We can continue working out the details in other PRs I think.