Last Comment Bug 775684 - rm PND_TOPLEVEL
: rm PND_TOPLEVEL
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 775323
  Show dependency treegraph
 
Reported: 2012-07-19 12:02 PDT by Luke Wagner [:luke]
Modified: 2012-08-05 13:43 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm (10.31 KB, patch)
2012-07-19 12:02 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-19 12:02:20 PDT
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] 2012-07-19 12:02:41 PDT
Created attachment 643953 [details] [diff] [review]
rm
Comment 2 Eddy Bruel [:ejpbruel] 2012-07-23 06:07:26 PDT
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] 2012-07-23 09:29:11 PDT
(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 2012-07-23 10:55:04 PDT
(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 6 Ed Morley [:emorley] 2012-07-24 03:01:37 PDT
https://hg.mozilla.org/mozilla-central/rev/527ee767d0b6

Note You need to log in before you can comment on or make changes to this bug.