src: migrate to new V8 array API by kt3k · Pull Request #24613 · nodejs/node

kt3k · 2018-11-24T07:58:03Z

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

This change migrates the deprecated V8 Array API to new APIs.

addaleax · 2018-11-24T08:01:14Z

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

TimothyGu · 2018-11-25T18:36:57Z

src/node_util.cc


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


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

Then way not ~~simply~~:

Suggested change

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);

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

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?

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...)

I followed @TimothyGu's comment for the moment.

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).

This addresses the comment in nodejs#24613

Use static sized data structure

kt3k · 2018-11-28T13:10:26Z

(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 · 2018-11-28T13:20:27Z

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

gireeshpunathil · 2018-11-28T14:16:34Z

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

Trott · 2018-11-29T03:14:20Z

Landed in 27139fc

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 · 2018-11-29T03:21:33Z

Thank you for the reviews! Thank you for landing!

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>

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>

src: migrate to new V8 array API

b3a0e1f

This change migrates the deprecated V8 Array API to new APIs.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Nov 24, 2018

addaleax approved these changes Nov 24, 2018

View reviewed changes

addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 24, 2018

joyeecheung approved these changes Nov 24, 2018

View reviewed changes

cjihrig approved these changes Nov 24, 2018

View reviewed changes

gireeshpunathil approved these changes Nov 25, 2018

View reviewed changes

devsnek approved these changes Nov 25, 2018

View reviewed changes

TimothyGu approved these changes Nov 25, 2018

View reviewed changes

src: set maximum of vector

11230d2

This addresses the comment in nodejs#24613

kt3k force-pushed the feature/node-util-improvement branch 2 times, most recently from fe987df to 0098eb2 Compare November 28, 2018 04:57

src: refactor src/node_util.cc

3729328

Use static sized data structure

kt3k force-pushed the feature/node-util-improvement branch from 0098eb2 to 3729328 Compare November 28, 2018 12:50

joyeecheung approved these changes Nov 28, 2018

View reviewed changes

Trott closed this Nov 29, 2018

kt3k deleted the feature/node-util-improvement branch November 29, 2018 03:21

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

This was referenced Dec 7, 2018

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262

Closed

BethGriggs added the dont-land-on-v10.x label Feb 12, 2019

-  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);

Uh oh!

Conversation

kt3k commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax commented Nov 24, 2018

Uh oh!

TimothyGu Nov 25, 2018

Choose a reason for hiding this comment

Uh oh!

refack Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

refack Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

kt3k Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

kt3k Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

kt3k Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

kt3k commented Nov 28, 2018

Uh oh!

joyeecheung commented Nov 28, 2018

Uh oh!

gireeshpunathil commented Nov 28, 2018

Uh oh!

Trott commented Nov 29, 2018

Uh oh!

kt3k commented Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

kt3k commented Nov 24, 2018 •

edited

Loading

refack Nov 25, 2018 •

edited

Loading