Last Comment Bug 778919 - remove CheckStrictParameters
: remove CheckStrictParameters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: 775323
  Show dependency treegraph
 
Reported: 2012-07-30 13:54 PDT by Luke Wagner [:luke]
Modified: 2012-08-09 04:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move DefineArg and BindDestructuringArg to be next to functionArguments (7.24 KB, patch)
2012-07-30 13:55 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
do CheckStrictBindings like everywhere else (4.59 KB, patch)
2012-07-30 13:56 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
rm CheckStrictParameters (5.02 KB, patch)
2012-07-30 13:58 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-07-30 13:54:03 PDT
CheckStrictParameters is kindof an oddball:
 - it iterates over all bindings (not just parameters, why this is ok takes some hand-waving)
 - it builds a hashset containing all formals, to look for duplicates (even though we already have tc->decls)
 - it checks for the bad names (eval, keywords), which we usually do when defining bindings and already do for destructuring formals

This weirdness gets in the way of bug 775323.

This trio of patches removes it by putting the relevant checks into DefineArg and BindDestructuringArg, making them more symetrics with the rest of the parser's handling of strict-mode checks.
Comment 1 Luke Wagner [:luke] 2012-07-30 13:55:45 PDT
Created attachment 647286 [details] [diff] [review]
move DefineArg and BindDestructuringArg to be next to functionArguments

Simple syntax movement + a comment.
Comment 2 Luke Wagner [:luke] 2012-07-30 13:56:57 PDT
Created attachment 647287 [details] [diff] [review]
do CheckStrictBindings like everywhere else
Comment 3 Luke Wagner [:luke] 2012-07-30 13:58:15 PDT
Created attachment 647289 [details] [diff] [review]
rm CheckStrictParameters

Do the dup-checking at the one use site.  Strict mode makes this an error, so the bool to check whether we've already warned was useless.
Comment 4 Eddy Bruel [:ejpbruel] 2012-07-31 08:56:13 PDT
Comment on attachment 647287 [details] [diff] [review]
do CheckStrictBindings like everywhere else

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

I assume your comment that:
- it checks for the bad names (eval, keywords), which we usually do when defining bindings and already do for destructuring formals"

was wrong, because you had to add a call to CheckStrictBinding in BindDestructuringArg?

Other than that, looks to me like this patch does the right thing.

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