-
Notifications
You must be signed in to change notification settings - Fork 291
Builtins for arbitrary precision integer arithmetic #5852
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
Conversation
…rorama/bigintnat # Conflicts: # unison-src/transcripts-using-base/all-base-hashes.output.md # unison-src/transcripts/idempotent/alias-term.md # unison-src/transcripts/idempotent/alias-type.md # unison-src/transcripts/idempotent/branch-squash.md # unison-src/transcripts/idempotent/builtins-merge.md # unison-src/transcripts/idempotent/emptyCodebase.md # unison-src/transcripts/idempotent/fix1532.md # unison-src/transcripts/idempotent/lib-install-local.md # unison-src/transcripts/idempotent/mcp.md # unison-src/transcripts/idempotent/move-all.md # unison-src/transcripts/idempotent/reflog.md # unison-src/transcripts/idempotent/reset.md # unison-src/transcripts/idempotent/undo.md # unison-src/transcripts/idempotent/upgrade.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good morning — what is this file? It looks like html but is has .txt extension. Was it checked in intentionally?
aryairani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but @dolio do you wanna take a peek?
pchiusano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! We can always add new literal syntax later if we want.
Minor thing: Could you change names to Integer and Natural? Integer is the type of integers, plain and simple, there's nothing big about it.
I know the builtin names don't actually matter but I'd rather the names be consistent with what they end up being called in base. And I like "Integer" and "Natural" so much better.
dolio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good to me.
# Conflicts: # unison-src/transcripts/idempotent/alias-term.md # unison-src/transcripts/idempotent/alias-type.md # unison-src/transcripts/idempotent/builtins-merge.md # unison-src/transcripts/idempotent/emptyCodebase.md # unison-src/transcripts/idempotent/fix1532.md # unison-src/transcripts/idempotent/lib-install-local.md # unison-src/transcripts/idempotent/mcp.md # unison-src/transcripts/idempotent/move-all.md # unison-src/transcripts/idempotent/undo.md # unison-src/transcripts/idempotent/upgrade.md
|
Builtin names do matter, which is why I didn't pick Not to worry, the plan is for the names of these types to be |
Yeah this sounds like a bug. |
|
@pchiusano Sorry I missed your comment about builtin names when merging; I saw Dan's "looks good" and hit the button. If you spot anything that shouldn't go into a release (settling on a builtin name qualifies IMO), don't hesitate to use the "Request Changes" review feature, to draw more attention to it. |
|
We can still change the names before the next release |
|
Yeah, I'd change it in a quick follow up. Otherwise you are going to see I don't think inability to give type signatures that mention |
|
I don't think ##BigInt is a terrible name though in any way; it's established in other libraries. But yeah I only meant that we should settle on the name before the release. |
This PR adds
##BigIntand#BigNatprimitives and operations on them.Controversial decisions
This is implemented via a foreign convention to the
Numeric.NaturalandIntegerHaskell types. It could have alternatively have been implemented by expanding the core Unison term language, which would require updating the runtime machinery as well. I feel that what I've done is a simpler solution, though perhaps not as flexible. For example it's more work to add a syntax for these types later if we want one.