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

N api dataview #14382

wants to merge 10 commits into from

Conversation

shivanth
Copy link
Contributor

@shivanth shivanth commented Jul 20, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

n-api: Add support for DataView …Basic support for Dataview is added by this commit. This is achieved by using three functions, napi_create_dataview(), napi_is_dataview() and napi_get_dataview_info().

Fixes: #13926

(edited by @vsemozhetbyt: fix formatting

Basic support for Dataview is added by this commit. This is achieved
by using three functions, napi_create_dataview(), napi_is_dataview()
and napi_get_dataview_info();

Fixes: nodejs#13926
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 20, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 20, 2017

It seems there are two similar napi_get_typedarray_info sections now (in the second one, the signature is out of the code block).

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good to me! I only have a few comments left.

doc/api/n-api.md Outdated

#### *napi_create_dataview*
<!-- YAML
added: v8.0.0
Copy link
Member

Choose a reason for hiding this comment

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

added: REPLACEME

doc/api/n-api.md Outdated
<!-- YAML
added: v8.0.0
-->
```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?

doc/api/n-api.md Outdated
size_t length,
napi_value arraybuffer,
size_t byte_offset,
napi_value* result)
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 align the arguments vertically (i.e. size_t length directly below napi_env env etc.?)

doc/api/n-api.md Outdated
- `[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.
Copy link
Member

Choose a reason for hiding this comment

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

typo: DataView (also, I’d indent this line to be on the same indentation as `[in] …)

doc/api/n-api.md Outdated
added: v8.0.0
-->
```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

doc/api/n-api.md Outdated
- `[out] length`: Number of elements in the DataView.
- `[out] data`: The data buffer underlying the DataView.
- `[out] byte_offset`: The byte offset within the data buffer from which
to start projecting the DataView.
Copy link
Member

Choose a reason for hiding this comment

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

Is documentation for arraybuffer missing?

doc/api/n-api.md Outdated
This API returns various properties of a DataView.

*Warning*: Use caution while using this API since the underlying data buffer
is managed by the VM
Copy link
Member

Choose a reason for hiding this comment

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

I don’t really know what this is saying? If you do use the napi_value* arraybuffer parameter, this should be okay, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds right

doc/api/n-api.md Outdated
<!-- YAML
added: v8.0.0
-->
```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)

doc/api/n-api.md Outdated

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.

I’d keep spelling it DataView

src/node_api.cc Outdated
size_t length,
napi_value arraybuffer,
size_t byte_offset,
napi_value* result) {
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 align the parameters here as well?

doc/api/n-api.md Outdated


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.

doc/api/n-api.md Outdated

### *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.

src/node_api.h Outdated
@@ -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.


//create dataview
const buffer3 = new ArrayBuffer(128);
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 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?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with comments.

doc/api/n-api.md Outdated


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.

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

doc/api/n-api.md Outdated
@@ -3169,6 +3251,7 @@ support it:
[Working with JavaScript Properties]: #n_api_working_with_javascript_properties
[Working with JavaScript Values]: #n_api_working_with_javascript_values
[Working with JavaScript Values - Abstract Operations]: #n_api_working_with_javascript_values_abstract_operations
[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 this up a few lines to keep the links sorted. This should come after the Section 12.5.5 link.

doc/api/n-api.md Outdated

```C
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.

doc/api/n-api.md Outdated
Dataview objects provide an array-like view over an underlying data buffer,
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.

doc/api/n-api.md Outdated
```C
napi_status napi_get_dataview_info(napi_env env,
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.


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 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_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 :)

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Almost there!

doc/api/n-api.md Outdated
Dataview objects provide an array-like view over an underlying data buffer,
but one which allows items of different size and type in the ArrayBuffer.

It's required that (byte_length * size_of_element) + 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.

size_of_element is always 1 for DataView. Delete the * size_of_element part plesse

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.

Reminder to whoever lands this: extra space in front.

@TimothyGu
Copy link
Member

LGTM. Thanks @shivanth!

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 26, 2017
@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/11385/

This should be ready now, if CI comes back good and this isn’t landed tomorrow, please bump this thread. :)

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Sorry Anna 😉


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"
Copy link
Member

Choose a reason for hiding this comment

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

Spelling: argments, and an additional space between dataview and as.
As dataview refers to a type here, I think DataView would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Also, there is no space between first and argument..


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.

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

Dataview objects provide an array-like view over an underlying data buffer,
but one which allows items of different size and type in the ArrayBuffer.

It's required that byte_length + 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.

Maybe put byte_length + byte_offset in backticks? Also, I think we prefer formal language (It is) over shortness.

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

It's required that byte_length + byte_offset
should be <= the size in bytes of the array passed in. If not, a RangeError
Copy link
Member

Choose a reason for hiding this comment

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

"It is required that ... should be" does not make sense in my head (I am not a native English speaker though).


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.

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.

Spelling: arugments.
As dataview refers to a type here, I think DataView would be better.

Copy link
Member

@tniessen tniessen Jul 26, 2017

Choose a reason for hiding this comment

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

Also, this could just be the same message as above.

};

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.

Missing space after exports,.

DECLARE_NAPI_PROPERTY("CreateDataView", CreateDataView)
};

NAPI_CALL_RETURN_VOID(env,napi_define_properties(
Copy link
Member

Choose a reason for hiding this comment

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

Missing space after env,.

tniessen pushed a commit that referenced this pull request Jul 27, 2017
Basic support for Dataview is added in this commit. This is achieved
by using three functions, napi_create_dataview(), napi_is_dataview()
and napi_get_dataview_info().

PR-URL: #14382
Fixes: #13926
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@tniessen
Copy link
Member

Landed in 77ca3cb with my changes addressed, thank you for your first contribution @shivanth! 🎉 CI and linter pass on master.

@addaleax surprise! 😛

@tniessen tniessen closed this Jul 27, 2017
addaleax pushed a commit that referenced this pull request Jul 27, 2017
Basic support for Dataview is added in this commit. This is achieved
by using three functions, napi_create_dataview(), napi_is_dataview()
and napi_get_dataview_info().

PR-URL: #14382
Fixes: #13926
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Basic support for Dataview is added in this commit. This is achieved
by using three functions, napi_create_dataview(), napi_is_dataview()
and napi_get_dataview_info().

PR-URL: nodejs#14382
Fixes: nodejs#13926
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Basic support for Dataview is added in this commit. This is achieved
by using three functions, napi_create_dataview(), napi_is_dataview()
and napi_get_dataview_info().

Backport-PR-URL: #19447
PR-URL: #14382
Fixes: #13926
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N-API: Support for DataView
8 participants