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

Add js BigInts #16409

Merged
merged 84 commits into from
Jan 14, 2021
Merged

Add js BigInts #16409

merged 84 commits into from
Jan 14, 2021

Conversation

juancarlospaco
Copy link
Collaborator

@ringabout
Copy link
Member

ringabout commented Dec 20, 2020

We really need bigInt(JS backend) in compiler
See:
nim-lang/RFCs#187
#13298

@juancarlospaco
Copy link
Collaborator Author

juancarlospaco commented Dec 20, 2020

I know and the C/C++ too, still stuff gotta start somewhere... 🤷

lib/js/jsbigint.nim Outdated Show resolved Hide resolved
@juancarlospaco juancarlospaco changed the title Add BigInt Add BigInts Dec 21, 2020
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
lib/js/jsbigints.nim Outdated Show resolved Hide resolved
Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent!

lib/std/jsbigints.nim Outdated Show resolved Hide resolved
lib/std/jsbigints.nim Outdated Show resolved Hide resolved
lib/std/jsbigints.nim Outdated Show resolved Hide resolved
lib/std/jsbigints.nim Outdated Show resolved Hide resolved
lib/std/jsbigints.nim Outdated Show resolved Hide resolved
lib/std/jsbigints.nim Outdated Show resolved Hide resolved
Copy link
Member

@ringabout ringabout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after addressing some comments.

@timotheecour
Copy link
Member

timotheecour commented Jan 13, 2021

@Araq friendly ping on this, is there anything else needed? from my perspective every blocker was addressed, and the rest can be done in future PRs

@Araq Araq merged commit 4196588 into nim-lang:devel Jan 14, 2021
@timotheecour
Copy link
Member

@juancarlospaco what was the reason again for:

func `**`*(x, y: JsBigInt): JsBigInt {.importjs: "((#) $1 #)".}
# instead of
func `**`*(x, y: JsBigInt): JsBigInt {.importjs: "(# $1 #)".}

?
IIRC you mentioned the reason but I can't remember; the 2nd form seems to work at least here:

import std/jsbigints
proc main()=
  block:
    var a = big"2"
    var b = big"3"
    var c = a ** b
    echo c
  block:
    var c = [big"2"]
    echo c[0] ** c[0]
main()

this is also useful for the description of P3 in nim-lang/RFCs#315

@juancarlospaco
Copy link
Collaborator Author

The operator is introvert and shy, and likes to be surrounded by parenthesis.

#16409 (comment)

@timotheecour
Copy link
Member

timotheecour commented Jan 14, 2021

thanks, this helped.

here's an even clearer explanation: the problem is when there's a unary operator
(EDIT)

func `**`*(x, y: int): int {.importjs: "((#) ** #)".}
func `***`*(x, y: int): auto = (x, y)
func `****`*(x, y: int): int {.importjs: "(# ** #)".}

proc id(a: auto): auto = a

doAssert id(-2 *** 3) == (-2, 3) # checks that - has higher precedence than ***
doAssert id(-2 ** 3) == -8 # ok
doAssert id(`****`(2, 3)) == 8
doAssert id(`****`(1+1, 3)) == 8

doAssert id(-2 **** 3) == -8 #  # js error: SyntaxError: Unary operator used immediately before exponentiation expression. Parenthesis must be used to disambiguate operator precedence
# doAssert id((-2) **** 3) == -8 # ditto
# doAssert id(`****`((-2), 3)) == -8 # ditto

note

for func ***(x, y: JsBigInt): JsBigInt {.importjs: "((#) $1 #)".}, the (#) is also needed but it's tricker to see why:

# suppose someone adds this other proc that doesn't have ():
func neg(this: JsBigInt): JsBigInt {.importjs: "-#".}
let b = big"1"
echo (-b ** big"3") # works
echo (b.neg ** big"3") # js error

@timotheecour
Copy link
Member

@juancarlospaco 2nd question: you're using outer parens in all your importjs procs (eg: importjs: "(# + #)". Is that actually required, and can you think of an example where ommitting those would result in incorrect code? I couldn't find such example yet

@timotheecour
Copy link
Member

@juancarlospaco also, followup PR welcome for missing procs (eg: bitwise and, or) as mentioned by @xflywind

@ringabout
Copy link
Member

Ah, I already make that PR :)
#16843

@timotheecour timotheecour mentioned this pull request Mar 13, 2021
2 tasks
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* Add BigInts
* Renames tos plurals
* Improve Stringifications
* Update changelog.md

Co-authored-by: flywind <[email protected]>

* RunnableExamplerize
* discard the discardable pragma
* Several improvements from peer reviews, more docs
* More doc, more test
* More doc, more test
* Better error message 'Error: usage of low is an {.error.} defined at jsbigints.nim' instead of just 'type mismatch JsBigInt'
* is an overload, rename
* proc to scare kids away
* Update lib/js/jsbigints.nim

Co-authored-by: Timothee Cour <[email protected]>

* nim-lang#16409 (comment)

Co-authored-by: flywind <[email protected]>
Co-authored-by: Timothee Cour <[email protected]>
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.

4 participants