Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: terpri, Assigned: terpri)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
Implement the StringToBigInt operation, used by the BigInt constructor and in the parser.
(Assignee)

Updated

a year ago
Depends on: js-bigint
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
(Assignee)

Comment 2

11 months ago
Attachment #8981359 - Flags: review?(jdemooij)
(Assignee)

Updated

11 months ago
Attachment #8975216 - Attachment is obsolete: true
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
Attachment #8981359 - Flags: review?(jdemooij)
(Assignee)

Comment 4

11 months ago
- 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
Attachment #8982326 - Flags: review?(jdemooij)
(Assignee)

Updated

11 months ago
Attachment #8981359 - Attachment is obsolete: true
Comment on attachment 8982326 [details] [diff] [review]
Implement string-to-BigInt conversion.

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

Nice, thanks!
Attachment #8982326 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 6

11 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08753da56e12
Implement string-to-BigInt conversion. r=jandem
Keywords: checkin-needed
Blocks: js-bigint
No longer depends on: js-bigint

Comment 7

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08753da56e12
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → robin
You need to log in before you can comment on or make changes to this bug.