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

Finalize implementation of the multiplication routines #18

Closed
AaronKutch opened this issue May 15, 2018 · 10 comments
Closed

Finalize implementation of the multiplication routines #18

AaronKutch opened this issue May 15, 2018 · 10 comments

Comments

@AaronKutch
Copy link
Contributor

Before I submit a pull request, should there be another branch or do I just use the master branch?

@Robbepop
Copy link
Owner

Ideally you implement new features on another branch than master since you then can use master to stay in sync with the origin/upstream master. It is just convention but if everybody sticks to conventions everything is easier.

@AaronKutch
Copy link
Contributor Author

I attempted to do a pull request but there is a mixture of spaces and tabs being used which messes up the indentation. Do you want to use tabs because that's what it looks like you are using?

@Robbepop
Copy link
Owner

I personally use what is the default in the ecosystem.

I viewed your pull request but you have closed it already. You can git push --force your fork to overwrite bad states. This will automatically update the current pull request on GitHub. So you do not need to close it. Just re-open your PR, fix the stuff locally, push --force to your branch on GitHub and everything will be fine again. :)

Just one little question: Why have you used macros and macro invokations instead of simple plain functions?

@Robbepop
Copy link
Owner

I have recently looked into what is required by the implementation of karatsuba algorithm. It seems to be easily implementable but let's see. It is good to have your initial base implementation for medium size ApInt instances first so that I can check both implementations against each other later on.

@AaronKutch
Copy link
Contributor Author

AaronKutch commented May 17, 2018

VScode, git, bugs, my internet connection, and more have been conspiring against me recently and preventing me from uploading stuff. During that time I have almost completed the division algorithm, and I did some code clean up.

I removed the macros since it turns out that the division algorithm needs a specialized type and they should have been functions anyway.

I saw a bunch of stuff in your code like

  let lval = lhs.repr();
  let rval = rhs.repr();
  let result = lval.wrapping_sub(rval);
  *lhs = Digit(result);

and I am replacing it with *lhs = lhs.wrapping_sub(rhs); since I have added those helper methods for my mul and div routines.

I am also fixing negate by adding a wrapping_inc function (that just increments by 1).

That also reminds me that there needs to be a renaming done. I think I will comment more about that after everything is done with this pull request

@Robbepop
Copy link
Owner

Thank you really really much for all the work you have put in here!
I am looking forward to merging your changes.

In general I really like the refactoring of old code and it is okay to me to include it into this PR but in the future it would be awesome if we could package Pull Requests into single pieces of new features instead of bundling them into a huge pile of feature additions and refactorings. This at least tends to keep things simpler and more transparent.

The wrapping_inc method is a nice addition but I was not implementing it since I wasn't sure if we want such an API or if something like wrapping_inc_by(int) is more appropriate. But since you have already implemented it, we will simply go with it now. ;)

Could we please do the renaming decoupled from this Pull-Request?
It seems that this Pull Request is already going to be a lot larger than a single closed-up feature addition or bug fix.

@AaronKutch
Copy link
Contributor Author

Ok I was planning on doing the renaming another pull request

@AaronKutch
Copy link
Contributor Author

Before this issue is closed, I want to get back to my project and try out some calculations that use the long multiplication.

@Robbepop
Copy link
Owner

Robbepop commented May 21, 2018

That's a good idea! :) I might do the same.
I will release the new version once we close this issue. :)

@Robbepop Robbepop changed the title implementing unimplemented! in multiplication functions Finalize implementation of the multiplication routines Oct 30, 2018
@AaronKutch
Copy link
Contributor Author

Because of out-of-date information here, I am continuing this in a new issue

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

No branches or pull requests

2 participants