-
-
Notifications
You must be signed in to change notification settings - Fork 408
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 newTarget to construct #1045
Conversation
Test262 conformance changes:
|
Codecov Report
@@ Coverage Diff @@
## master #1045 +/- ##
==========================================
+ Coverage 58.37% 58.59% +0.22%
==========================================
Files 173 173
Lines 12050 12214 +164
==========================================
+ Hits 7034 7157 +123
- Misses 5016 5057 +41
Continue to review full report at Codecov.
|
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.
This is looking very good. I had some comments, though. Let me know if you think they make sense :)
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 some minor changes to improve readability, and minor bugs.
I'm not sure using the first argument for new_target
when normally it would be for this
is correct (I'd like to get @HalidOdat's opinion on this). There's FunctionEnvironmentRecord.new_target
, but that's not really used...
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.
Code looks good, still unsure about having newTarget
be the first argument of constructors.
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'm ok having newTarget
as first argument for now, that can be changed later if we need to or find a better solution. I'm happy to merge this :)
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.
Looks good to me! :)
This Pull Request fixes #1009.
It changes the following:
construct
method to handlenew_target
,new_target
String
constructor to make it more compliant