1169511 - Split PNK_TYPEOF into one kind for application to names, one kind for application to other expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

PNK_DELETE emboldened me to finish the job and deal with the other single-kind-that-should-be-many.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8612682 [details] [diff] [review]
Patch

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

r=me with comment about unifying the emitter functions answered.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7012,5 @@
> +    return emit1(JSOP_TYPEOF);
> +}
> +
> +bool
> +BytecodeEmitter::emitTypeofExpr(ParseNode* node)

bool
BytecodeEmitter::emitTypeof(ParseNode* pn, JSOp op)
{
    MOZ_ASSERT(op == JSOP_TYPEOF || op == JSOP_TYPEOFEXPR);

    <Identical code from both methods>

    return emit1(op);
}

Are we building towards making these do different things, or something?

@@ +7495,5 @@
>          break;
>        }
>  
> +      case PNK_TYPEOFNAME:
> +        ok = emitTypeofName(pn);

we can do this |ok =| business, but Jason and I were trying to make new ones |return emitTypeofName(pn)|. What do you think?

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4275a4bf842e

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Eric Faust [:efaust] from comment #2)
> > +bool
> > +BytecodeEmitter::emitTypeofExpr(ParseNode* node)
> 
> bool
> BytecodeEmitter::emitTypeof(ParseNode* pn, JSOp op)
> {
>     MOZ_ASSERT(op == JSOP_TYPEOF || op == JSOP_TYPEOFEXPR);
> 
>     <Identical code from both methods>
> 
>     return emit1(op);
> }
> 
> Are we building towards making these do different things, or something?

I think so, yes.  That JSOP_TYPEOF and JSOP_TYPEOFEXPR share the same code is pretty bonkers, versus having separate code paths.  But for another day.  I unified for now, it's easy to split these later.

> > +      case PNK_TYPEOFNAME:
> > +        ok = emitTypeofName(pn);
> 
> we can do this |ok =| business, but Jason and I were trying to make new ones
> |return emitTypeofName(pn)|. What do you think?

The code after the overall switch doesn't obviously not apply to these cases, so I left this as-is.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out so I could cleanly back out bug 1141865 for bustage. Feel free to reland this as soon as I reopen the tree.

Comment 6

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7772a97fafcf

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d86e23a06839

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d86e23a06839