775684 - rm PND_TOPLEVEL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

This flag has only one obscure use in BindNameToSlot: turning 'delete x' on a top-level declaration 'x' into 'false' (non-top-level defs mean 'eval', whose defs are deletable).  Since this should be a very cold path, I think the right way to handle this is to just make 'delete some-name' force heavyweight and JSOP_DELNAME which already handle this correctly at runtime.

The reason for this is to make Define better match BindVarOrConst in how it mutates the ParseNode for the purposes of bug 775323.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm

Comment 2

[Eddy Bruel [:ejpbruel]]

Comment on attachment 643953 [details] [diff] [review]
rm

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

If I understand this patch correctly, making the function heavyweight ensures that delete returns the proper result. It should return false when trying to delete a non-global property, but can only do so if the name to be deleted is found on the scope object. Setting bindings to be accessed dynamically ensures that the name to be deleted actually appears on the scope object, since it won't be noted as closed over in the emitter.

However, it looks to me as if using setBindingsAccessedDynamically is overly conservative here. Since names can only be deleted explicitly, we could keep track of the set of names that is mentioned in a delete, and mark any definition nodes in tc->decls matching those names as closed over when leaving the current function. Then again, since this is probably a very cold path, this might not be worth the effort.

Otherwise, this patch looks good to me.

Comment 3

[Luke Wagner [:luke]]

(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> However, it looks to me as if using setBindingsAccessedDynamically is overly
> conservative here. 

You're right.

> Then again, since this is probably a very cold
> path, this might not be worth the effort.

That was my conclusion as well.

Comment 4

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> However, it looks to me as if using setBindingsAccessedDynamically is overly
> conservative here. Since names can only be deleted explicitly, we could keep
> track of the set of names that is mentioned in a delete, and mark any
> definition nodes in tc->decls matching those names as closed over when
> leaving the current function. Then again, since this is probably a very cold
> path, this might not be worth the effort.

That's a great observation. Although, I'm happier that we're not going to try this approach; having the "aliased" flag also mean "potential operand of delete" is kind of excruciating.

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/527ee767d0b6

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/527ee767d0b6