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

src: migrate to new V8 array API #24613

Closed
wants to merge 3 commits into from
Closed

src: migrate to new V8 array API #24613

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3a0e1f
src: migrate to new V8 array API
kt3k Nov 24, 2018
11230d2
src: set maximum of vector
kt3k Nov 27, 2018
3729328
src: refactor src/node_util.cc
kt3k Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,15 @@ static void GetPromiseDetails(const FunctionCallbackInfo<Value>& args) {
auto isolate = args.GetIsolate();

Local<Promise> promise = args[0].As<Promise>();
Local<Array> ret = Array::New(isolate, 2);

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@refack refack Nov 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed @TimothyGu's comment for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

values.push_back(Integer::New(isolate, state));
if (state != Promise::PromiseState::kPending)
ret->Set(env->context(), 1, promise->Result()).FromJust();
values.push_back(promise->Result());

Local<Array> ret = Array::New(
isolate, values.data(), values.size());
args.GetReturnValue().Set(ret);
}

Expand All @@ -82,11 +84,13 @@ static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {

Local<Proxy> proxy = args[0].As<Proxy>();

Local<Array> ret = Array::New(args.GetIsolate(), 2);
ret->Set(env->context(), 0, proxy->GetTarget()).FromJust();
ret->Set(env->context(), 1, proxy->GetHandler()).FromJust();
Local<Value> ret[] = {
proxy->GetTarget(),
proxy->GetHandler()
};

args.GetReturnValue().Set(ret);
args.GetReturnValue().Set(
Array::New(args.GetIsolate(), ret, arraysize(ret)));
}

static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
Expand All @@ -101,11 +105,13 @@ static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
// Fast path for WeakMap, WeakSet and Set iterators.
if (args.Length() == 1)
return args.GetReturnValue().Set(entries);
Local<Array> ret = Array::New(env->isolate(), 2);
ret->Set(env->context(), 0, entries).FromJust();
ret->Set(env->context(), 1, Boolean::New(env->isolate(), is_key_value))
.FromJust();
return args.GetReturnValue().Set(ret);

Local<Value> ret[] = {
entries,
Boolean::New(env->isolate(), is_key_value)
};
return args.GetReturnValue().Set(
Array::New(env->isolate(), ret, arraysize(ret)));
}

// Side effect-free stringification that will never throw exceptions.
Expand Down