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

DON'T MERGE YET - NEED TO MEASURE CONSTRAINTS chore: Refactor #27

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

iAmMichaelConnor
Copy link
Collaborator

Description

I spent a bit of time refactoring BigNum into what I hope is an easier-to-follow design.

I haven't touched any of the mathematics; just moved functions around.

Things I sought to improve:

  • The BigNum and RuntimeBigNum were somewhat cyclic structs: their methods called each-others' methods. The comments in the code even mentioned this being problematic and confusing. It also meant if you tried to introduce a new method (like I tried), you could accidentally fall afoul of the cyclic-ness.
    • To solve this, I've put made all of the proper "impl" functions as "free functions" instead of methods. Both structs still exists. It's just their methods are now clean wrappers which call the free functions. This way, both structs can call those methods without any cyclic dependencies on each-other. In fact, the structs now don't know about each other at all.
  • When creating a BigNum from a BigNumInstance, many of the methods of BigNum weren't available (I think because of the above cyclic problems), and the pretty operators (+,-,*,/) weren't available.
    • I think this is because really, runtime bignum is a distinct thing that deserves its own struct. So, I introduced "RuntimeBigNum", which has exactly the same methods and pretty operators as BigNum. The only difference is whether the params are known at compile-time or not.
  • The above simplifications meant I could remove lots of traits and generic parameters which made the code difficult to follow. Hopefully it reads more cleanly.
  • Broke big files into a few smaller files.
  • Removed the "dummy struct" that needed to be declared when defining a new set of runtime bignum params.

Anyway, all the tests pass, wooh!

Lmk what you think. :)

@iAmMichaelConnor iAmMichaelConnor changed the title Refactor refactor: Refactor Oct 3, 2024
@iAmMichaelConnor
Copy link
Collaborator Author

Looks like nightly is happy, but 0.34.0 isn't. Will investigate.

@iAmMichaelConnor iAmMichaelConnor changed the title refactor: Refactor chore: Refactor Oct 3, 2024
@iAmMichaelConnor
Copy link
Collaborator Author

Before:
image

After:
image

@iAmMichaelConnor
Copy link
Collaborator Author

If approved, I'll do the README

src/utils/map.nr Outdated Show resolved Hide resolved
src/runtime_bignum.nr Outdated Show resolved Hide resolved
@iAmMichaelConnor iAmMichaelConnor changed the title chore: Refactor DON'T MERGE YET - NEED TO MEASURE CONSTRAINTS chore: Refactor Oct 4, 2024
@TomAFrench TomAFrench mentioned this pull request Oct 4, 2024
2 tasks
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.

2 participants