-
-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add default parameter support #1273
Conversation
Test262 conformance changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted a few things that are a bit off.
I'll give a more thorough look comparing with the spec, but the numbers seem to indicate that it's great.
Benchmark for a84880fClick to view benchmark
|
Benchmark for 342aa2cClick to view benchmark
|
Benchmark for 2eb9f23Click to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I have a couple of questions / suggestions.
This seems to fix #1062. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about something, other than that it looks good.
Co-authored-by: Iban Eguia <razican@protonmail.ch>
Only a partial fix - the issue doesn't happen when default parameters are used since |
Benchmark for e02d2f7Click to view benchmark
|
Okay, I added arguments object handling as defined in the spec, so this should fix #1062 properly now. |
Benchmark for 51e56f7Click to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just like for the comment to be more complete to avoid confusion in the future.
Benchmark for 9598615Click to view benchmark
|
@@ -72,6 +72,16 @@ impl StatementList { | |||
set | |||
} | |||
|
|||
pub fn function_declared_names(&self) -> HashSet<&str> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a bit of documentation to this function, to explain what it does.
This PR is a rewrite of #988, implementing default parameter support and merging
call
andconstruct
implementations into a single internal method.Modern JavaScript has support for default parameters in functions. Boa has support for the syntax but currently default parameters are ignored. For example, this code:
Result produced by current Boa master:
Expected result: