remove CheckStrictParameters

RESOLVED FIXED in mozilla17

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

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 647286 [details] [diff] [review]
move DefineArg and BindDestructuringArg to be next to functionArguments

Simple syntax movement + a comment.
Attachment #647286 - Flags: review?(ejpbruel)
(Assignee)

Comment 2

5 years ago
Created attachment 647287 [details] [diff] [review]
do CheckStrictBindings like everywhere else
Attachment #647287 - Flags: review?(ejpbruel)
(Assignee)

Comment 3

5 years ago
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.
Attachment #647289 - Flags: review?(ejpbruel)

Updated

5 years ago
Attachment #647286 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #647286 - Flags: review?(ejpbruel)
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.
Attachment #647287 - Flags: review+

Updated

5 years ago
Attachment #647289 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #647287 - Flags: review?(ejpbruel)
(Assignee)

Updated

5 years ago
Attachment #647289 - Flags: review?(ejpbruel)
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3055bc1c4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e60512aa511f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8a5b7e0d31
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/7f3055bc1c4c
https://hg.mozilla.org/mozilla-central/rev/e60512aa511f
https://hg.mozilla.org/mozilla-central/rev/ee8a5b7e0d31
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.