Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 643953 [details] [diff] [review]
rm
Attachment #643953 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
Attachment #643953 - Flags: review?(jimb) → review?(ejpbruel)
Whiteboard: [js:t]
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.
Attachment #643953 - Flags: review+
(Assignee)

Comment 3

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/527ee767d0b6
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/527ee767d0b6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #643953 - Flags: review?(ejpbruel)
You need to log in before you can comment on or make changes to this bug.