Skip to content

Conversation

ds26gte
Copy link
Contributor

@ds26gte ds26gte commented Sep 20, 2025

This in-progress PR addresses Issue #1812.

A JS test file tests/jsnums-test/jsnums-test.c will test the methods and functions defined in js-numbers.js (Pyret's number library) that cannot be adequately tested by Pyret test files such as tests/pyret/tests/test-numbers.js.

This PR is WIP, and not ready to merge yet.

expect(JN.fromString("1e140", sampleErrorBacks)).toEqual(JN.makeBignum("1e140"));

// for large bignums (> 1e140 ?), fromString() and makeBignum() can give structurally
// unequal results. so the following fail:

Choose a reason for hiding this comment

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

@ds26gte why are these commented out? We should be testing to make sure they fail, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that they should not fail, but were. I kept them around so I could solve the underlying problem as to why they were failing.

Now that I've canonicalized bigint representation -- i.e, every bigint now has a only one representation, not multiple as before --, these tests no longer fail, and are now uncommented.

- ensure integerNthRoot() is monotonically nondecreasing brownplt#1822
- require integerNthRoot() root and radicand to be non-neg
- require nthRoot() root to be non-neg
** should remove fields at .t and above
** should remove any trailing zero fields, also reducing .t
- bnDivide() should call clamp()
- test: bnToString _integerIsZero _integerIsOne _integerGcd
- _integer{Is{Zero,NegativeOne},Modulo,DivideToFixnum,{Greater,Less}Than{,OrEqual}}
- splitIntIntoMantissaExpt
- bnpD{L,R}ShiftTo() should canonicalize
- test for: makeInteger{UnOp,Binop}
- test for: bnpAddTo bnpDMultiply bnpMillerRabin
- test for: bnRemainder bnModPow bnDivideAndRemainder bnIsProbablePrime
@ds26gte
Copy link
Contributor Author

ds26gte commented Sep 29, 2025

tests/jsnums-test/jsnums-test.js now includes 328 (!) tests for the functions defined in src/js/base/js-numbers.js. This includes tests for functions that are either not available to test as Pyret programs, or have some calls that are not exercised by Pyret programs.

While adding tests, found the following corrections that needed to be applied to js-numbers.js:

  • liftFixnumInteger() — despite name, make it accept all fixnums, not just fixnum ints. (This is because it is possible to leak non-int fixnums into this pipeline, and the rational number objects thus created are not only bogus but cause silent errors. Alternative is to make it error on non-ints.)

  • isInteger() — should return false on non-int fixnums. (Alternative is to make it error on non-int fixnums, which doesn't seem right)

  • numerator(), denominator(), remainder(), toStringDigits(), equals(), lessThan(), lessThanOrEqual() — propragate errbacks argument

  • Rational.*.numerator(), Rational.*.denominator(), Roughnum.*.numerator(), Roughnum.*.denominator(), BigInteger.*.numerator(), BigInteger.*.denominator() — should all take errbacks parameter

  • makeIntegerBinop(), makeIntegerUnOp — input & output functions should both take errbacks parameter

  • integerNthRoot() — should error on negative arguments. Initial guess should always be integer, or output is not monotonically non-decreasing

  • nthRoot() — should error on negative argument

  • bnpClamp() — ensure all unwanted fields are deleted, as also all non-significant 0-value fields. (This ensures unique representation for bigintegers — cleaner but also makes equality checks efficient)

  • bnpDLShiftTo(), bnpDRShiftTo(), bnDivide() — should use canonicalizer bnpClamp()

  • Returned module includes _innards field, which gives access to private functions so we can write JS tests for them

  • gcd(), lcm() simplified as we don't need to variadic arguments. Pyret num-gcd (as suggested by @blerner in add gcd operation? are the signatures the way we want? #1427), num-lcm defined

@ds26gte ds26gte self-assigned this Sep 29, 2025
@ds26gte ds26gte requested review from jpolitz and blerner September 29, 2025 13:21
- runtime.js: gcd, lcm simplified as variable num args unneeded
- add num-lcm also
- image-lib.js: use simplified gcd
- tests for num-gcd, num-lcm
@blerner
Copy link
Member

blerner commented Oct 6, 2025

N.B.: This is an incomplete review, done on my phone while waiting for my plane...

There are a lot of commented-out console.logs still in here, that probably ought to be cleaned up.

There's a call to r.clamp() in there somewhere whose purpose I don't understand - could you add a comment?

There's a comment in the tests wondering why toFixnum doesn't take errbacks -- but fromFixnum does. Why the asymmetry, and does it matter? (Do/could you have a test for toFixnum whose argument isn't a valid convertible number, to trigger a domain error?)

@ds26gte
Copy link
Contributor Author

ds26gte commented Oct 7, 2025

N.B.: This is an incomplete review, done on my phone while waiting for my plane...

There are a lot of commented-out console.logs still in here, that probably ought to be cleaned up.

There's a call to r.clamp() in there somewhere whose purpose I don't understand - could you add a comment?

There's a comment in the tests wondering why toFixnum doesn't take errbacks -- but fromFixnum does. Why the asymmetry, and does it matter? (Do/could you have a test for toFixnum whose argument isn't a valid convertible number, to trigger a domain error?)

I've scrubbed all the comments containing debug console.logs.

r.clamp() should be called (implicitly or explicitly) as the final step in all methods that produce a BigInteger, to ensure that its canonical form is used. This wasn't being done uniformly. (I also improved the definition of bnpClamp() so that it does produce a truly canonical form.)

Turns out toFixnum() doesn't need errbacks. toFixnum()does use_integerDivideToFixnum()— which is created usingmakeIntegerBinop()— for a Rational argument. (This is the only use of_integerDivideToFixnum().) makeIntegerBinop()`-generated functions could potentially need errbacks. In our code they're never used on roughnums, which is the only case where an overflow could occur. Before this PR, this (never occurring) situation used an errbacks argument that was not available as a parameter. Since this is dead code, a simple JS error rather than errbacks may be more appropriate here.

- document them both better
- test for both for limit < size(repeating-decimal)
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.

3 participants