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.

Sign up for GitHub

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

Jump to bottom

src: prefer 3-argument Array::New() #31775

Closed
wants to merge 2 commits into from
Closed

src: prefer 3-argument Array::New() #31775

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

This is nicer, because:

  1. It reduces overall code size,
  2. It’s faster, because Object::Set() calls are relatively slow, and
  3. It helps avoid invalid .Check()/.FromJust() calls.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax
src: prefer 3-argument Array::New()
d942aa9 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 13, 2020
@nodejs-github-bot
Copy link
Collaborator

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

bnoordhuis
bnoordhuis approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Nice!

src/module_wrap.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@addaleax @bnoordhuis
fixup! src: prefer 3-argument Array::New()
72c5cf9 
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 14, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member Author

Landed in a7c523e

@addaleax addaleax closed this Feb 15, 2020
addaleax added a commit that referenced this pull request Feb 15, 2020
@addaleax
src: prefer 3-argument Array::New()
a7c523e 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.

PR-URL: #31775
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax addaleax deleted the array-new-3 branch February 15, 2020 20:40
@sthagen sthagen mentioned this pull request Feb 16, 2020
Merged
4 tasks
codebytere pushed a commit that referenced this pull request Feb 27, 2020
@addaleax @codebytere
src: prefer 3-argument Array::New()
57302f8 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.

PR-URL: #31775
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@addaleax @codebytere
src: prefer 3-argument Array::New()
a998c16 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.

PR-URL: #31775
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@addaleax @codebytere
src: prefer 3-argument Array::New()
7eb9770 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.

PR-URL: #31775
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@addaleax @codebytere
src: prefer 3-argument Array::New()
e6debf5 
This is nicer, because:

1. It reduces overall code size,
2. It’s faster, because `Object::Set()` calls are relatively slow, and
3. It helps avoid invalid `.Check()`/`.FromJust()` calls.

PR-URL: #31775
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @bnoordhuis @jasnell @cjihrig @devnexen