836616 - allow ParseNodes to distinguish numbers containing a decimal point

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

This is useful for asm.js, which "types" numeric literals based on the presence/absence of a decimal point.

The patch tracks this extra bit of information.  Perhaps a better/more general approach would be to provide a mapping from a literal ParseNode back to its exact chars.  However the "index" in a ParseNode is a per-line thing and I don't know of a simple way to map from index to offset in the char buffer.  Do you Jason?

Comment 1

[Nicholas Nethercote [inactive]]

> Perhaps a better/more
> general approach would be to provide a mapping from a literal ParseNode back
> to its exact chars.  However the "index" in a ParseNode is a per-line thing
> and I don't know of a simple way to map from index to offset in the char
> buffer.

I don't think there is one.  I tried to convert the line/index pairs to offsets a while back, but it was kinda painful and had some performance issues (esp. with "//@line" lines) and never was finished.  See bug 747831 for details.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 708436 [details] [diff] [review]
patch

Ah, thanks for info Nick.  I forgot, you did major work on the scanner; perhaps I could punt this to you?

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 708436 [details] [diff] [review]
patch

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

Nice.  I actually like this approach more than having a pointer back to the original literal (which would just have to be re-parsed in order to see if a '.' is present).

::: js/src/frontend/ParseNode.h
@@ +625,5 @@
>          } name;
> +        struct {
> +            double      value;          /* aligned numeric literal value */
> +            bool        hasFrac;        /* whether the literal contains . */
> +        } number;

I double-checked (mentally) that this doesn't increase the size of ParseNode :)

::: js/src/frontend/TokenStream.cpp
@@ +1439,5 @@
>      Token *tp;
>      FirstCharKind c1kind;
>      const jschar *numStart;
> +    bool hasFrac;
> +    bool hasExp;

You could do |bool hasFrac, hasExp;| if you wanted.

@@ +1595,5 @@
>          c = getCharIgnoreEOL();
>          if (JS7_ISDEC(c)) {
>              numStart = userbuf.addressOfNextRawChar() - 2;
> +            hasFrac = true;
> +            hasExp = false;

Man, the assignment and control flow surrounding these variables is tragic, but I can't see how to make it better.

@@ +1782,5 @@
>           * convert the jschars in userbuf directly to the numeric value.
>           */
>          double dval;
>          const jschar *dummy;
> +        if (!(hasFrac | hasExp)) {

|| instead of |.

@@ +1874,5 @@
>          double dval;
>          const jschar *dummy;
>          if (!GetPrefixInteger(cx, numStart, userbuf.addressOfNextRawChar(), radix, &dummy, &dval))
>              goto error;
> +        tp->setNumber(dval, false);

I like writing |/* hasFrac = */ false| in cases like this.  Alternatively, you could make the 2nd arg default to |false|.

Comment 4

[Jeff Walden [:Waldo]]

> @@ +1874,5 @@
> >          double dval;
> >          const jschar *dummy;
> >          if (!GetPrefixInteger(cx, numStart, userbuf.addressOfNextRawChar(), radix, &dummy, &dval))
> >              goto error;
> > +        tp->setNumber(dval, false);
> 
> I like writing |/* hasFrac = */ false| in cases like this.  Alternatively,
> you could make the 2nd arg default to |false|.

enum DecimalPoint { HasFraction, Integral }; or something like that, named however's sensible.  "Enums should be preferred to boolean arguments for ease of understanding at the invocation site." as our style rules say, and this seems like one of those cases.

Comment 5

[Luke Wagner [:luke]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
I know, I was being lazy ;)  I'll enum-ify.

Comment 6

[Luke Wagner [:luke]]

Attachment: patch, with enum

Adding the enum ended up implying a bunch of nameing changes.  njn does it still look fine to you?

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 708802 [details] [diff] [review]
patch, with enum

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

If your two alternatives are called "HasX" and "NoX", IMO an enum isn't adding value.  Especially when the variable is sitting right next to another bool.  But I can live with this.

Comment 8

[Luke Wagner [:luke]]

The advantage to the enum, which is what Waldo was pointing out in comment 4, is that "setNumber(dval, NoDecimal)" is more suggestive than "setNumber(dval, false)".  Not a huge deal which is why I didn't fuss with it initially, but I do agree very much with the general style guideline of "naming" bools that get passed as parameters (recalling the horror days of attemptExtendTree(f, pc, op, NULL, 0, false, false, NULL, true) ;).

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c73235f1aa2

Comment 9

[Nicholas Nethercote [inactive]]

Probably a better alternative (too late, sorry) would be to get rid of the enum and then have setNumberNoDecimal() and setNumberHasDecimal().  Oh well.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c73235f1aa2