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: migrate to new V8 array API #24613

Closed
wants to merge 3 commits into
base: master
from

Conversation

@kt3k
Copy link
Contributor

kt3k commented Nov 24, 2018

This PR uses new V8 API for array creation, instead of old one.

related PR: #24125

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
src: migrate to new V8 array API
This change migrates the deprecated V8 Array API to new APIs.
@addaleax

This comment has been minimized.


int state = promise->State();
ret->Set(env->context(), 0, Integer::New(isolate, state)).FromJust();
std::vector<Local<Value>> values;

This comment has been minimized.

@TimothyGu

TimothyGu Nov 25, 2018

Member

values(2), as we can have a maximum of two values.

This comment has been minimized.

@refack

refack Nov 25, 2018

Member

Then way not simply:

Suggested change Beta
std::vector<Local<Value>> values;
Local<Value> values[2] = { Integer::New(isolate, state) };
size_t number_of_values = 1;
if (state != Promise::PromiseState::kPending)
values[number_of_values++] = promise->Result();
DCHECK_LE(number_of_values, arraysize(values));
Local<Array> ret = Array::New(isolate, values, number_of_values);

This comment has been minimized.

@TimothyGu

TimothyGu Nov 26, 2018

Member

That’s much longer and doesn’t really provide too much of an advantage over using a vector.

This comment has been minimized.

@refack

refack Nov 26, 2018

Member

AFAICT it's only one line longer, and we get everything on the stack with no free-store allocations.
I thought that when possible, we prefer static sized data structures?

This comment has been minimized.

@kt3k

kt3k Nov 27, 2018

Author Contributor

I tried @refack's suggestion, but I couldn't find a reasonable way to include DCHECK_LE in this file. DCHECK_LE is defined in deps/v8/src/base/logging.h, and when I add #include "../deps/v8/src/base/logging.h" at the top, the compile doesn't seem passing (relative path inside logging.h doesn't seem resolving...)

This comment has been minimized.

@kt3k

kt3k Nov 27, 2018

Author Contributor

I followed @TimothyGu's comment for the moment.

This comment has been minimized.

@kt3k

kt3k Nov 28, 2018

Author Contributor

With values(2), the values are initialized with empty values and it has size 2. So I needed to add an else block and .pop_back() the last (empty) value. (I think that's unnecessarily complex and not desirable code.)

So I changed the code to follow @refack's comment (without DCHECK_LE assertion).

kt3k added a commit to kt3k/node that referenced this pull request Nov 27, 2018

src: set maximum of vector
This addresses the comment in nodejs#24613
src: set maximum of vector
This addresses the comment in #24613

@kt3k kt3k force-pushed the kt3k:feature/node-util-improvement branch 2 times, most recently from fe987df to 0098eb2 Nov 27, 2018

src: refactor src/node_util.cc
Use static sized data structure

@kt3k kt3k force-pushed the kt3k:feature/node-util-improvement branch from 0098eb2 to 3729328 Nov 28, 2018

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Nov 28, 2018

(I force-pushed with empty change a few times to fix CI status but it keeps failing because of the rate limit of github API, which is used for fetching the commit message.)

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 28, 2018

@kt3k We don't have our own DCHECK macros - it's being added in #24359

@gireeshpunathil

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 29, 2018

Landed in 27139fc

@Trott Trott closed this Nov 29, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 29, 2018

src: migrate to new V8 array API
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: nodejs#24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Nov 29, 2018

Thank you for the reviews! Thank you for landing!

@kt3k kt3k deleted the kt3k:feature/node-util-improvement branch Nov 29, 2018

targos added a commit that referenced this pull request Nov 29, 2018

src: migrate to new V8 array API
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: #24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

src: migrate to new V8 array API
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: nodejs#24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment