1461050 - Implement StringToBigInt

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Robin Templeton [:terpri]]

Implement the StringToBigInt operation, used by the BigInt constructor and in the parser.

Comment 1

[Robin Templeton [:terpri]]

Attachment: Implement string-to-BigInt conversion.

Comment 2

[Robin Templeton [:terpri]]

Attachment: Implement string-to-BigInt conversion and use it in ToBigInt.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8981359 [details] [diff] [review]
Implement string-to-BigInt conversion and use it in ToBigInt.

Review of attachment 8981359 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but some minor issues and nits below.

::: js/src/vm/BigIntType.cpp
@@ +163,5 @@
> +        RootedString str(cx, v.toString());
> +        RootedBigInt bi(cx, StringToBigInt(cx, str, 0));
> +        if (!bi) {
> +            JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
> +                                      JSMSG_BIGINT_INVALID_SYNTAX);

StringToBigInt can also report nullptr on OOM (from ensureLinear) so it would be invalid to report this in that case.

I'd suggest moving this error reporting into StringToBigInt, that's also more consistent with SpiderMonkey code elsewhere.

@@ +195,5 @@
> +bool
> +js::StringToBigIntImpl(const CharT* chars, size_t length, unsigned radix,
> +                       HandleBigInt res)
> +{
> +    const CharT* end = chars + length;

It might be nice to pass |const Range<const CharT>& chars| instead of chars and length arguments. Caller can pass linear->{latin1,twoByte}Range(nogc).

Then here you can do:

  const RangedPtr<const CharT> end = chars.end();
  RangedPtr<const CharT> s = chars.begin();

That will give us some bounds checking in debug builds.

@@ +199,5 @@
> +    const CharT* end = chars + length;
> +    const CharT* s = SkipSpace(chars, end);
> +
> +    bool hasSign = false;
> +    int8_t sign = 0;

Nit: here it could be nice to use

  mozilla::Maybe<int8_t> sign;

Then sign.emplace(1) etc.

@@ +269,5 @@
> +    JS::AutoCheckCannotGC nogc;
> +    size_t length = str->length();
> +    if (linear->hasLatin1Chars()) {
> +        if (!StringToBigIntImpl(linear->latin1Chars(nogc), length, radix, res)) {
> +            return nullptr;

Nit: no {} here and below

Comment 4

[Robin Templeton [:terpri]]

Attachment: Implement string-to-BigInt conversion.

- change StringToBigIntImpl to use Range/RangedPtr for chars and Maybe
   for sign
 - move syntax error reporting to StringToBigInt
 - make StringToBigInt's radix argument uint8_t for consistency with
   BigInt::toString

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8982326 [details] [diff] [review]
Implement string-to-BigInt conversion.

Review of attachment 8982326 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08753da56e12
Implement string-to-BigInt conversion. r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/08753da56e12