Skip to content
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

Fixes for NPM test breaks. #4659

Merged
merged 1 commit into from
Feb 9, 2018
Merged

Conversation

MSLaguana
Copy link
Contributor

Some node-chakracore tests were throwing asserts when run with TTD.

Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally fine to me. If possible, I think I'd prefer to store the new.target value in a separate field so it's not confusing down the line but I can accept this as-is.


if (args.HasNewTarget())
{
callEvt->ArgArray[args.Info.Count + 1] = static_cast<TTDVar>(args.GetNewTarget());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to allocate one more pointer in callEvt to handle the new.target (callEvt->newTargetPointer?) instead of tacking it on at the end? This is our data structure so we don't really need to sneakily add an extra arg here and it might be confusing why the argument count doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrkmarron can you have a look at the change I made for this?

Copy link
Contributor

@mike-kaufman mike-kaufman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mrkmarron
Copy link
Contributor

lgtm

@chakrabot chakrabot merged commit 6dd42b0 into chakra-core:release/1.9 Feb 9, 2018
chakrabot pushed a commit that referenced this pull request Feb 9, 2018
Merge pull request #4659 from MSLaguana:fixTTD

Some node-chakracore tests were throwing asserts when run with TTD.
@MSLaguana MSLaguana deleted the fixTTD branch February 9, 2018 05:27
chakrabot pushed a commit that referenced this pull request Feb 9, 2018
Merge pull request #4659 from MSLaguana:fixTTD

Some node-chakracore tests were throwing asserts when run with TTD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants