1706866 - Add more embedder API for BigInt

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Philip Chimento [:ptomato]]

Follow-up from bug 1606568.

In GJS we'd find a few more BigInt APIs useful to have available for embedders. (More info: https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/524)

I'd propose to add the following APIs in js/public/BigInt.h:

/**
 * Convert the given BigInt to a Number value as if calling the Number
 * constructor on it
 * (https://tc39.es/ecma262/#sec-number-constructor-number-value). The value
 * may be rounded if it doesn't fit without loss of precision.
 */
extern JS_PUBLIC_API double BigIntToNumber(BigInt* bi);

/**
 * Return true if the given BigInt is negative.
 */
extern JS_PUBLIC_API bool BigIntIsNegative(BigInt* bi);

/**
 * Return true if the given BigInt fits inside the given NumericT type without
 * loss of precision, and store the value in the out parameter. Otherwise return
 * false and leave the value of the out parameter unspecified.
 */
template <typename NumericT, std::enable_if_t<std::is_integral_v<UnsignedIntT>>>
extern JS_PUBLIC_API bool BigIntFits(BigInt* bi, NumericT* out);

/**
 * Same as BigIntFits(), but checks if the value fits inside a JS Number value.
 */
extern JS_PUBLIC_API bool BigIntFitsNumber(BigInt* bi, double* out);

/**
 * Convert the given BigInt to a String value as if toString() were called on
 * it.
 */
extern JS_PUBLIC_API JSString* BigIntToString(JSContext* cx, BigInt* bi);

#if defined(DEBUG) || defined(JS_JITSPEW)
/**
 * Print the given BigInt to stderr, useful for debuggers.
 */
extern JS_PUBLIC_API void DumpBigInt(BigInt* bi);
#endif

Does this look like a good plan? Bikesheds welcome on the names and forms of the APIs.

Could also add the arithmetic operations as suggested in the original bug.

I would be willing to work on this and would like to get it in for ESR 91.

Comment 1

[Marco Trevisan (Treviño)]

Thanks for proposing this!

extern JS_PUBLIC_API bool BigIntIsNegative(BigInt* bi);

Wouldn't be nice to have an operator overloading for this?

Ideally, since we have BigIntFromInt64 and BigIntFromUint64 would be also nice to know if a bigint has been generated from one of such types, and so whether it was originally signed or not.

Not sure it matters much, but if known could make us respect the type when converting things back and forth.

Comment 2

[Philip Chimento [:ptomato]]

Attachment: Bug 1706866 - Change radix type to uint8_t in JS::SimpleStringToBigInt(). r?jandem

There are actually several places in vm/BigIntType.h where unsigned is
used for the radix, and several other places where uint8_t is used. In the
public API, prefer the narrower type, for consistency with the API that
will be added in the next commit.

https://bugzilla.mozilla.org/show_bug.cgi?id=1706866

Comment 3

[Philip Chimento [:ptomato]]

Attachment: Bug 1706866 - Add more BigInt API for embedders. r?jandem

New APIs:

• JS::BigIntToNumber() - convert to JS Number as if calling Number(x)
• JS::BigIntIsNegative() - return true if value < 0n
• JS::BigIntFits() - return true and fill outparam if value fits in the
  given integer type
• JS::BigIntFitsNumber() - return true and fill outparam if value fits in
  a JS Number
• JS::BigIntToString() - convert to a string as if toString() called
• js::DumpBigInt() - debug-only, dump to stderr or a FILE*

Adds jsapi-tests for all of these APIs as well, except for DumpBigInt().
Requires adding two new private static methods, BigInt::isUint64() and
BigInt::isNumber(), which do approximately the same as BigInt::isInt64().

https://bugzilla.mozilla.org/show_bug.cgi?id=1706866

Depends on D117191

Comment 4

[Philip Chimento [:ptomato]]

Attachment: Bug 1706866 - Fix up various details of existing BigInt public API. r?jandem

• Change the digits limit on the signed-int specialization of
  NumberToBigIntConverter to 63, not 64, because
  std::numeric_limits<int64_t>::digits == 63
• Make the inline template function static inline, not JS_PUBLIC_API

These changes are in order to align with the new API being added.

https://bugzilla.mozilla.org/show_bug.cgi?id=1706866

Depends on D117192

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2ace7587c0d
Change radix type to uint8_t in JS::SimpleStringToBigInt(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/4ea62a9df2cb
Add more BigInt API for embedders. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d0807cbb6a58
Fix up various details of existing BigInt public API. r=jandem

Comment 6

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/c2ace7587c0d
https://hg.mozilla.org/mozilla-central/rev/4ea62a9df2cb
https://hg.mozilla.org/mozilla-central/rev/d0807cbb6a58