Closed Bug 836616 Opened 10 years ago Closed 10 years ago

allow ParseNodes to distinguish numbers containing a decimal point

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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?
Attachment #708436 - Flags: review?(jorendorff)
> 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 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?
Attachment #708436 - Flags: review?(jorendorff) → review?(n.nethercote)
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|.
Attachment #708436 - Flags: review?(n.nethercote) → review+
> @@ +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.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
I know, I was being lazy ;)  I'll enum-ify.
Attached patch patch, with enumSplinter Review
Adding the enum ended up implying a bunch of nameing changes.  njn does it still look fine to you?
Attachment #708436 - Attachment is obsolete: true
Attachment #708802 - Flags: review?(n.nethercote)
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.
Attachment #708802 - Flags: review?(n.nethercote) → review+
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
Probably a better alternative (too late, sorry) would be to get rid of the enum and then have setNumberNoDecimal() and setNumberHasDecimal().  Oh well.
https://hg.mozilla.org/mozilla-central/rev/4c73235f1aa2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.