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

N api dataview #14382

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
82 changes: 82 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,39 @@ JavaScript TypedArray Objects are described in
[Section 22.2](https://tc39.github.io/ecma262/#sec-typedarray-objects)
of the ECMAScript Language Specification.


#### *napi_create_dataview*
<!-- YAML
added: REPLACEME
-->

```C
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a newline before this?

napi_status napi_create_dataview(napi_env env,
size_t length,
Copy link
Member

Choose a reason for hiding this comment

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

length should be byte_length for clarity.

napi_value arraybuffer,
size_t byte_offset,
napi_value* result)

```

- `[in] env`: The environment that the API is invoked under.
- `[in] length`: Number of elements in the DataView.
- `[in] arraybuffer`: ArrayBuffer underlying the DataView.
- `[in] byte_offset`: The byte offset within the ArrayBuffer from which to
start projecting the DataView.
- `[out] result`: A `napi_value` representing a JavaScript DataView.

Returns `napi_ok` if the API succeeded.

This API creates a JavaScript DataView object over an existing ArrayBuffer.
Dataview objects provide an array-like view over an underlying data buffer,
Copy link
Member

Choose a reason for hiding this comment

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

Capitalization: Dataview

but one which allows items of different size and type in the ArrayBuffer.


Copy link
Member

Choose a reason for hiding this comment

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

napi_create_typedarray() has

It's required that (length * size_of_element) + byte_offset should be <= the size in bytes of the array passed in. If not, a RangeError exception is raised.

There should one here as well, with "(length * size_of_element)" replaced with byte_length.

JavaScript DataView Objects are described in
[Section 24.3](https://tc39.github.io/ecma262/#sec-dataview-objects)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the link to the bottom of the file.

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 see that every napi_create_x function refers to the ECMA standards within it's section through out the document. Shouldn't we stick to this format ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This should now just be [Section 24.3][]. Search the file for Section 12.5.5 for an example.

of the ECMAScript Language Specification.

### Functions to convert from C types to N-API
#### *napi_create_number*
<!-- YAML
Expand Down Expand Up @@ -1552,6 +1585,36 @@ This API returns various properties of a typed array.
*Warning*: Use caution while using this API since the underlying data buffer
is managed by the VM



#### *napi_get_dataview_info*
<!-- YAML
added: REPLACEME
-->

```C
napi_status napi_get_dataview_info(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

same comments here

napi_value dataview,
size_t* bytelength,
Copy link
Member

Choose a reason for hiding this comment

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

byte_length for consistency with other functions.

void** data,
napi_value* arraybuffer,
size_t* byte_offset)
```

- `[in] env`: The environment that the API is invoked under.
- `[in] dataview`: `napi_value` representing the DataView whose
properties to query.
- `[out] bytelength`: Number of bytes in the DataView.
- `[out] data`: The data buffer underlying the DataView.
- `[out] arraybuffer`: ArrayBuffer underlying the DataView.
- `[out] byte_offset`: The byte offset within the data buffer from which
to start projecting the DataView.

Returns `napi_ok` if the API succeeded.

This API returns various properties of a DataView.


#### *napi_get_value_bool*
<!-- YAML
added: v8.0.0
Expand Down Expand Up @@ -2019,6 +2082,25 @@ Returns `napi_ok` if the API succeeded.

This API checks if the Object passsed in is a typed array.



### *napi_is_dataview*
<!-- YAML
added: v8.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

REPLACEME here too please.

-->

```C
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)

napi_status napi_is_dataview(napi_env env, napi_value value, bool* result)
```

- `[in] env`: The environment that the API is invoked under.
- `[in] value`: The JavaScript value to check.
- `[out] result`: Whether the given `napi_value` represents a DataView.

Returns `napi_ok` if the API succeeded.

This API checks if the Object passsed in is a DataView.
Copy link
Member

Choose a reason for hiding this comment

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

Spelling: passsed.


### *napi_strict_equals*
<!-- YAML
added: v8.0.0
Expand Down
67 changes: 67 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2957,6 +2957,73 @@ napi_status napi_get_typedarray_info(napi_env env,
return napi_clear_last_error(env);
}

napi_status napi_create_dataview(napi_env env,
size_t length,
napi_value arraybuffer,
size_t byte_offset,
napi_value* result) {
NAPI_PREAMBLE(env);
CHECK_ARG(env, arraybuffer);
CHECK_ARG(env, result);

v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(arraybuffer);
RETURN_STATUS_IF_FALSE(env, value->IsArrayBuffer(), napi_invalid_arg);

v8::Local<v8::ArrayBuffer> buffer = value.As<v8::ArrayBuffer>();
v8::Local<v8::DataView> DataView = v8::DataView::New(buffer, byte_offset,
length);

*result = v8impl::JsValueFromV8LocalValue(DataView);
return GET_RETURN_STATUS(env);
}

napi_status napi_is_dataview(napi_env env, napi_value value, bool* result) {
CHECK_ENV(env);
CHECK_ARG(env, value);
CHECK_ARG(env, result);

v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
*result = val->IsDataView();

return napi_clear_last_error(env);
}

napi_status napi_get_dataview_info(napi_env env,
napi_value dataview,
size_t* bytelength,
void** data,
napi_value* arraybuffer,
size_t* byte_offset) {
CHECK_ENV(env);
CHECK_ARG(env, dataview);

v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(dataview);
RETURN_STATUS_IF_FALSE(env, value->IsDataView(), napi_invalid_arg);

v8::Local<v8::DataView> array = value.As<v8::DataView>();


Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would try to avoid multiple consecutive empty lines within functions. It usually does not improve readability.

if (bytelength != nullptr) {
*bytelength = array->ByteLength();
}

v8::Local<v8::ArrayBuffer> buffer = array->Buffer();
if (data != nullptr) {
*data = static_cast<uint8_t*>(buffer->GetContents().Data()) +
array->ByteOffset();
}

if (arraybuffer != nullptr) {
*arraybuffer = v8impl::JsValueFromV8LocalValue(buffer);
}

if (byte_offset != nullptr) {
*byte_offset = array->ByteOffset();
}

return napi_clear_last_error(env);
}

napi_status napi_get_version(napi_env env, uint32_t* result) {
CHECK_ENV(env);
CHECK_ARG(env, result);
Expand Down
15 changes: 15 additions & 0 deletions src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,21 @@ NAPI_EXTERN napi_status napi_get_typedarray_info(napi_env env,
napi_value* arraybuffer,
size_t* byte_offset);

NAPI_EXTERN napi_status napi_create_dataview(napi_env env,
size_t length,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you line up the args please.

napi_value arraybuffer,
size_t byte_offset,
napi_value* result);
NAPI_EXTERN napi_status napi_is_dataview(napi_env env,
napi_value value,
bool* result);
NAPI_EXTERN napi_status napi_get_dataview_info(napi_env env,
napi_value dataview,
size_t* bytelength,
void** data,
napi_value* arraybuffer,
size_t* byte_offset);

// Methods to manage simple async operations
NAPI_EXTERN
napi_status napi_create_async_work(napi_env env,
Expand Down
8 changes: 8 additions & 0 deletions test/addons-napi/test_dataview/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"targets": [
{
"target_name": "test_dataview",
"sources": [ "test_dataview.c" ]
}
]
}
17 changes: 17 additions & 0 deletions test/addons-napi/test_dataview/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';
const common = require('../../common');
const assert = require('assert');

// Testing api calls for arrays
const test_dataview = require(`./build/${common.buildType}/test_dataview`);

//create dataview
const buffer3 = new ArrayBuffer(128);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this named buffer3?

console.log(buffer3 instanceof ArrayBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the console.log() calls.

const template = Reflect.construct(DataView, [buffer3]);
console.log(template);
console.log(template instanceof DataView);

const theDataview = test_dataview.CreateDataView(template);
assert.ok(theDataview instanceof DataView,
'The new variable should be of type Dataview');
45 changes: 45 additions & 0 deletions test/addons-napi/test_dataview/test_dataview.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include <node_api.h>
#include <string.h>
#include "../common.h"

napi_value CreateDataView(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args [1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NAPI_ASSERT(env, argc == 1, "Wrong number of arguments");

napi_valuetype valuetype;
napi_value input_dataview = args[0];

NAPI_CALL(env, napi_typeof(env, input_dataview, &valuetype));
NAPI_ASSERT(env, valuetype == napi_object,
"Wrong type of argments. Expects a dataview as the first argument.");
Copy link
Member

Choose a reason for hiding this comment

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

Align the arguments please, like so:

NAPI_ASSERT(env, valuetype == napi_object,
            "Wrong type of argument. Expects a DataView as the first "
            "argument.");


bool is_dataview;
NAPI_CALL(env, napi_is_dataview(env, input_dataview, &is_dataview));
NAPI_ASSERT(env,is_dataview,
"Wrong type of arugments. Expects a dataview as first argument.");
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

size_t byte_offset = 0;
size_t length = 0;
napi_value buffer;
NAPI_CALL(env, napi_get_dataview_info(
env, input_dataview, &length, NULL, &buffer, &byte_offset));
Copy link
Member

Choose a reason for hiding this comment

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

  NAPI_CALL(env,
            napi_get_dataview_info(env, input_dataview, &byte_length, NULL,
	                           &buffer, &byte_offset));


napi_value output_dataview;
NAPI_CALL(env, napi_create_dataview(
env, length, buffer, byte_offset, &output_dataview));

return output_dataview;
}

void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_PROPERTY("CreateDataView", CreateDataView)
};

NAPI_CALL_RETURN_VOID(env, napi_define_properties(
env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));
Copy link
Member

Choose a reason for hiding this comment

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

Four-space indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how do I format this 😁

Copy link
Member

@addaleax addaleax Jul 21, 2017

Choose a reason for hiding this comment

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

Just add two more spaces, so that env, … is indented four columns more than the beginning of the statement :)

}

NAPI_MODULE(addon, Init)