Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@targos
Copy link
Member

@targos targos commented Feb 9, 2016

I did not touch SetWeak and SetAccessor yet (will need help for those).

Ref: #4869

cc @nodejs/v8

@targos targos added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Feb 9, 2016
@bnoordhuis
Copy link
Member

LGTM with style suggestions. Good work.

@targos
Copy link
Member Author

targos commented Feb 10, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1625/

Should I squash everything into one commit (and list the changes in the message) before landing ?

@bnoordhuis
Copy link
Member

Whatever you prefer. I'd be okay with keeping the commits separate, might be easier if we need to bisect.

targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos added a commit that referenced this pull request Feb 11, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ofrobots pushed a commit that referenced this pull request Mar 4, 2016
PR-URL: #5159
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor

To confirm, is this safe to land in v5.x? (Please reply in #5559)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.