Closed Bug 836949 Opened 7 years ago Closed 7 years ago

e4x removal follow-ups

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: evilpie)

References

Details

(Whiteboard: [js:t][leave open])

Attachments

(5 files, 3 obsolete files)

Things I didn't want to tackle in bug 788293:

- PNX_FUNCDEFS and PNX_SETCALL have the same value.

- All the version/flags stuff is over-engineered, now.  E.g.:
  - remove JSCOMPILEOPTION_MASK
  - remove FULL_MASK
  - remove |copts| in jscntxt.h
  - remove OptionFlagsToVersion
  - etc

- PushNodeChildren: in the PN_NULLARY case, just return true.

- make_unicode.py needs to be updated and re-run.  evilpie, can you do this?
No longer depends on: 788293
Depends on: 788293
Attachment #709747 - Flags: review?(jwalden+bmo)
> - PushNodeChildren: in the PN_NULLARY case, just return true.

I got an assertion failure in jit-test/tests/auto-regress/bug601393.js with this change, so I won't do it.
"Runtime options" are now just "options", and "compile options" are gone.
Attachment #710064 - Flags: review?(jorendorff)
Attachment #710066 - Flags: review?(jorendorff)
OK but I'm calling dibs on deleting the unused source notes.
> OK but I'm calling dibs on deleting the unused source notes.

Sure thing!  I didn't realize we had ones to delete.  I was going to ask you to CC me on the bug, but instead I just filed it myself:  bug 838813.
Comment on attachment 710066 [details] [diff] [review]
(part 2) - Disambiguate PNX_FUNCDEFS and PNX_SETCALL.

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

According to bug 838813 comment 16, I have successfully ignored this long enough that I don't need to review it anymore.
Attachment #710066 - Flags: review?(jorendorff)
Comment on attachment 710064 [details] [diff] [review]
(part 1) - Clean up the versions/options code.

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

Very nice!
Attachment #710064 - Flags: review?(jorendorff) → review+
Attachment #710066 - Attachment is obsolete: true
Attachment #709747 - Attachment description: remove e4x only unicode flags → (part 2) - remove e4x only unicode flags
Attachment #710064 - Attachment description: (part 3) - Clean up the versions/options code. → (part 1) - Clean up the versions/options code.
Comment on attachment 709747 [details] [diff] [review]
(part 2) - remove e4x only unicode flags

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

Whew, fun times paging all this code back into memory.

::: js/src/vm/make_unicode.py
@@ +211,5 @@
>   * Step 2:
>   *  Using these bits to get an reduced index from index1.
>   *   index = index1[upper]
>   * Step 3:
> + *  Combining the index and the bottom 5 bits of the orginial jschar.

original

@@ +217,2 @@
>   *
>   * The advantage here is that the bigest number in index1 doesn't need 10 bits,

biggest

@@ +217,3 @@
>   *
>   * The advantage here is that the bigest number in index1 doesn't need 10 bits,
> + * but 7 and we save some memory.

I find myself wondering if we shouldn't make this output be generated based on |shift| directly.

"""all that comment""".format(shift = shift, shiftComplementStars = "*" * (16 - shift), shiftDots = "." * shift)

and so on.  But I guess this suffices to make it correct, tho.
Attachment #709747 - Flags: review?(jwalden+bmo) → review+
For that matter, I'm thinking probably we should generate both Unicode.h and Unicode.cpp, so that we can just always use the computed shift value, and never hard-code it anywhere.  This hardcoding, I note, has the potential to result in some UnicodeData.txt update causing the update script to break, if enough characters of interest got added in the right ways.  I think.  Right?  New bug to follow up on that if you agree.
These all have an extra |obj| parameter and some really strange semantics that I would rather get rid of.
Attachment #715553 - Flags: review?(jwalden+bmo)
Attachment #715554 - Flags: review?(jwalden+bmo)
Comment on attachment 715554 [details] [diff] [review]
(part 4) remove obj parameter from ValueToId

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

Stealing review.

::: js/src/jsinterp.cpp
@@ +1590,5 @@
>      }
>  }
>  END_CASE(JSOP_AND)
>  
> +#define FETCH_ELEMENT_ID(cx, n, id)                                           \

The macros in jsinterp.cpp are mostly unhygienic.  So there's not much point passing in |cx| here -- I'd just remove it.
Attachment #715554 - Flags: review?(jwalden+bmo) → review+
Attachment #709747 - Flags: checkin+
Comment on attachment 715553 [details] [diff] [review]
(part 3) - remove InternNonIntElementId/FetchElementId

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

r- for the BytecodeEmitter.cpp bit, otherwise fine enough.  Starting to use PropertyKey everywhere can't come soon enough!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3527,5 @@
> +                    return false;
> +                }
> +            }
> +
> +            JSAtom *name = ToAtom<CanGC>(cx, idvalue);

Erm, shouldn't everything from this ToAtom downward be in an else-block corresponding to the IsDefinitelyIndex call above?  If it's definitely an element, you're defining the element twice, no?
Attachment #715553 - Flags: review?(jwalden+bmo) → review-
Assignee: n.nethercote → evilpies
Attachment #710064 - Flags: checkin+
Attachment #715553 - Attachment is obsolete: true
Attachment #730699 - Flags: review?(jwalden+bmo)
Carrying r+.
Attachment #715554 - Attachment is obsolete: true
Attachment #730701 - Flags: review?(n.nethercote)
Attachment #730701 - Flags: review?(n.nethercote) → review+
Comment on attachment 730699 [details] [diff] [review]
(part 3) - remove InternNonIntElementId/FetchElementId  !v2

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3459,5 @@
>              if (!pn->pn_right->getConstantValue(cx, strictChecks, &value))
>                  return false;
>  
>              ParseNode *pnid = pn->pn_left;
> +            RootedValue idvalue(cx);

This is best put outside the loop, to avoid eventual push/pop overhead for hooking this up to the Rooted stack to track these.

@@ +3471,5 @@
> +
> +            uint32_t index;
> +            if (IsDefinitelyIndex(idvalue, &index)) {
> +                if (!JSObject::defineElement(cx, obj, index, value, NULL, NULL,
> +                                             JSPROP_ENUMERATE))

This looks like it'd fit in 100ch to me, but maybe you've actually measured.  :-)  Same for the other defineElement below.

::: js/src/jsatominlines.h
@@ +36,5 @@
> +    uint32_t index;
> +    if (atom->isIndex(&index) && index <= JSID_INT_MAX)
> +        return INT_TO_JSID((int32_t) index);
> +
> +    return JSID_FROM_BITS((size_t)atom);

Constructor-style casts here, please.

::: js/src/jsinterp.cpp
@@ +2936,5 @@
>  BEGIN_CASE(JSOP_INITELEM)
>  {
>      JS_ASSERT(regs.stackDepth() >= 3);
>      HandleValue val = HandleValue::fromMarkedLocation(&regs.sp[-1]);
> +    HandleValue id = MutableHandleValue::fromMarkedLocation(&regs.sp[-2]);

Not HandleValue::fromMarkedLocation?
Attachment #730699 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> Comment on attachment 730699 [details] [diff] [review]
> (part 3) - remove InternNonIntElementId/FetchElementId  !v2
> 
> Review of attachment 730699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +3459,5 @@
> >              if (!pn->pn_right->getConstantValue(cx, strictChecks, &value))
> >                  return false;
> >  
> >              ParseNode *pnid = pn->pn_left;
> > +            RootedValue idvalue(cx);
> 
> This is best put outside the loop, to avoid eventual push/pop overhead for
> hooking this up to the Rooted stack to track these.
> 
Good catch! I also moved |value| outside.
> @@ +3471,5 @@
> > +
> > +            uint32_t index;
> > +            if (IsDefinitelyIndex(idvalue, &index)) {
> > +                if (!JSObject::defineElement(cx, obj, index, value, NULL, NULL,
> > +                                             JSPROP_ENUMERATE))
> 
> This looks like it'd fit in 100ch to me, but maybe you've actually measured.
> :-)  Same for the other defineElement below.
> 
I made this change, however it looks kind of strange, because the defineElement doesn't fit.
> ::: js/src/jsatominlines.h
> @@ +36,5 @@
> > +    uint32_t index;
> > +    if (atom->isIndex(&index) && index <= JSID_INT_MAX)
> > +        return INT_TO_JSID((int32_t) index);
> > +
> > +    return JSID_FROM_BITS((size_t)atom);
> 
> Constructor-style casts here, please.
> 
> ::: js/src/jsinterp.cpp
> @@ +2936,5 @@
> >  BEGIN_CASE(JSOP_INITELEM)
> >  {
> >      JS_ASSERT(regs.stackDepth() >= 3);
> >      HandleValue val = HandleValue::fromMarkedLocation(&regs.sp[-1]);
> > +    HandleValue id = MutableHandleValue::fromMarkedLocation(&regs.sp[-2]);
> 
> Not HandleValue::fromMarkedLocation?
Hehe.
We used to have some special behavior when you wanted to get property as a method from e4x objects. This is now obviously gone.

I don't really have anything on my mind that would be easy to fix now. Any ideas left?
Attachment #742556 - Flags: review?(jwalden+bmo)
Comment on attachment 742556 [details] [diff] [review]
(part 5) Remove GetMethod

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

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1266,1 @@
>              // XXX We really want to factor out the error reporting better and

Why is this comment moving?  It looks like it applied to what happened if !JS_GetMethod(...), which would now be !JS_GetProperty(...), which would mean it shouldn't move, and the |thisObj = obj| assignment should be after it.
Attachment #742556 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 742556 [details] [diff] [review]
(part 5) Remove GetMethod

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

::: js/src/json.cpp
@@ +270,5 @@
>  
>      /* Step 2. */
>      if (vp.get().isObject()) {
>          RootedValue toJSON(cx);
> +        RootedObject obj(cx, &vp.get().toObject());

Do we really need those .get()s?
(In reply to Tom Schuster [:evilpie] from comment #25)
> I don't really have anything on my mind that would be easy to fix now. Any
> ideas left?

Nothing in particular springs to mind.

(In reply to :Ms2ger from comment #27)
> > +        RootedObject obj(cx, &vp.get().toObject());
> 
> Do we really need those .get()s?

No.  Possibly they precede our adding the Value interface bits to {Mutable,}Handle<Value>.
OK. I am going to close this now. We can open new bugs if we find something interesting.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.