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

src: refactor deprecated v8::String::NewFromTwoByte call #23803

Closed
wants to merge 1 commit into from

Conversation

RomainLanz
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 21, 2018
@RomainLanz RomainLanz force-pushed the fix/node_process branch 2 times, most recently from 872378f to 5e5684b Compare October 21, 2018 14:45
@targos targos added the wip Issues and PRs that are still a work in progress. label Oct 21, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM with nit

@@ -52,7 +52,9 @@ using v8::HeapStatistics;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I wouldn't add a using just for 2 instances. In this case I would keep it explicit

@targos targos removed the wip Issues and PRs that are still a work in progress. label Oct 21, 2018
refack
refack previously approved these changes Oct 21, 2018
@refack refack dismissed their stale review October 21, 2018 17:26

has comment

@refack
Copy link
Contributor

refack commented Oct 21, 2018

@RomainLanz Thank you for the contribution, and all the follow ups 🥇
Code looks good, only thing is our CI doesn't handle merge commits properly, so if you could please git squash and rebase all the commits together.

P.S. If you are having trouble doing that I could help. Also feel free to contact me if you need help in general.

@RomainLanz
Copy link
Contributor Author

Done @refack 👍

@refack
Copy link
Contributor

refack commented Oct 21, 2018

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 21, 2018
@addaleax
Copy link
Member

Landed in added1b 🎉

@addaleax addaleax closed this Oct 24, 2018
addaleax pushed a commit that referenced this pull request Oct 24, 2018
PR-URL: #23803
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 24, 2018
PR-URL: #23803
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
PR-URL: #23803
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23803
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23803
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants