N api dataview #14382

shivanth · 2017-07-20T06:37:01Z

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

vsemozhetbyt · 2017-07-20T06:57:32Z

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

addaleax

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

addaleax · 2017-07-20T14:24:48Z

doc/api/n-api.md

+
+#### *napi_create_dataview*
+<!-- YAML
+added: v8.0.0


added: REPLACEME

addaleax · 2017-07-20T14:24:58Z

doc/api/n-api.md

+<!-- YAML
+added: v8.0.0
+-->
+```C


Can you leave a newline before this?

addaleax · 2017-07-20T14:25:22Z

doc/api/n-api.md

+                                   size_t length,
+                                   napi_value arraybuffer,
+                                   size_t byte_offset,
+                                   napi_value* result)


Can you align the arguments vertically (i.e. size_t length directly below napi_env env etc.?)

addaleax · 2017-07-20T14:26:02Z

doc/api/n-api.md

+- `[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.


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

addaleax · 2017-07-20T14:32:40Z

doc/api/n-api.md

+added: v8.0.0
+-->
+```C
+napi_status napi_get_dataview_info(napi_env env,


same comments here

addaleax · 2017-07-20T14:33:42Z

doc/api/n-api.md

+- `[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.


Is documentation for arraybuffer missing?

addaleax · 2017-07-20T14:34:01Z

doc/api/n-api.md

+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


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

Sounds right

addaleax · 2017-07-20T14:34:13Z

doc/api/n-api.md

+<!-- YAML
+added: v8.0.0
+-->
+```C


addaleax · 2017-07-20T14:34:28Z

doc/api/n-api.md

+
+Returns `napi_ok` if the API succeeded.
+
+This API checks if the Object passsed in is a dataview.


I’d keep spelling it DataView

addaleax · 2017-07-20T14:34:40Z

src/node_api.cc

+                                   size_t length,
+                                   napi_value arraybuffer,
+                                   size_t byte_offset,
+                                   napi_value* result) {


Can you align the parameters here as well?

cjihrig · 2017-07-20T19:02:20Z

doc/api/n-api.md

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


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

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 ?

I just mean like this - https://github.com/nodejs/node/blame/43e105f6453d50bb667a028d8e85d1882818d16a/doc/api/n-api.md#L3166-L3167

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

cjihrig · 2017-07-20T19:03:54Z

doc/api/n-api.md

+
+### *napi_is_dataview*
+<!-- YAML
+added: v8.0.0


REPLACEME here too please.

cjihrig · 2017-07-20T19:08:21Z

src/node_api.h

@@ -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,


Can you line up the args please.

cjihrig · 2017-07-20T19:11:42Z

test/addons-napi/test_dataview/test.js

+
+//create dataview
+const buffer3 = new ArrayBuffer(128);
+console.log(buffer3 instanceof ArrayBuffer);


Can you remove the console.log() calls.

cjihrig · 2017-07-20T19:12:06Z

test/addons-napi/test_dataview/test.js

+const test_dataview = require(`./build/${common.buildType}/test_dataview`);
+
+//create dataview
+const buffer3 = new ArrayBuffer(128);


Why is this named buffer3?

cjihrig

LGTM with comments.

cjihrig · 2017-07-21T01:25:44Z

doc/api/n-api.md

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


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

cjihrig · 2017-07-21T01:26:27Z

doc/api/n-api.md

@@ -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


Can you move this up a few lines to keep the links sorted. This should come after the Section 12.5.5 link.

TimothyGu · 2017-07-21T06:14:42Z

doc/api/n-api.md

+
+```C
+napi_status napi_create_dataview(napi_env env,
+                                 size_t length,


length should be byte_length for clarity.

TimothyGu · 2017-07-21T06:15:29Z

doc/api/n-api.md

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


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.

TimothyGu · 2017-07-21T06:16:00Z

doc/api/n-api.md

+```C
+napi_status napi_get_dataview_info(napi_env env,
+                                   napi_value dataview,
+                                   size_t* bytelength,


byte_length for consistency with other functions.

TimothyGu · 2017-07-21T06:18:46Z

test/addons-napi/test_dataview/test_dataview.c

+
+  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.");


Align the arguments please, like so:

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

TimothyGu · 2017-07-21T06:18:51Z

test/addons-napi/test_dataview/test_dataview.c

+  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.");


TimothyGu · 2017-07-21T06:19:53Z

test/addons-napi/test_dataview/test_dataview.c

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


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

TimothyGu · 2017-07-21T06:20:14Z

test/addons-napi/test_dataview/test_dataview.c

+  };
+
+  NAPI_CALL_RETURN_VOID(env, napi_define_properties(
+    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));


Four-space indentation.

Not sure how do I format this 😁

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

TimothyGu

Almost there!

TimothyGu · 2017-07-21T14:47:35Z

doc/api/n-api.md

+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


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

TimothyGu · 2017-07-21T14:49:06Z

test/addons-napi/test_dataview/test_dataview.c

+  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.");


Reminder to whoever lands this: extra space in front.

TimothyGu · 2017-07-21T15:02:40Z

LGTM. Thanks @shivanth!

addaleax · 2017-07-26T18:15:05Z

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

tniessen

Sorry Anna 😉

tniessen · 2017-07-26T23:05:36Z

test/addons-napi/test_dataview/test_dataview.c

+
+  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"


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

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

tniessen · 2017-07-26T23:06:10Z

doc/api/n-api.md

+
+Returns `napi_ok` if the API succeeded.
+
+This API checks if the Object passsed in is a DataView.


Spelling: passsed.

tniessen · 2017-07-26T23:07:03Z

doc/api/n-api.md

+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,


Capitalization: Dataview

tniessen · 2017-07-26T23:07:55Z

doc/api/n-api.md

+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


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

tniessen · 2017-07-26T23:09:18Z

doc/api/n-api.md

+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


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

tniessen · 2017-07-26T23:11:50Z

src/node_api.cc

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


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

tniessen · 2017-07-26T23:15:25Z

test/addons-napi/test_dataview/test_dataview.c

+  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.");


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

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

tniessen · 2017-07-26T23:16:50Z

test/addons-napi/test_dataview/test_dataview.c

+  };
+
+  NAPI_CALL_RETURN_VOID(env,napi_define_properties(
+      env, exports,sizeof(descriptors) / sizeof(*descriptors), descriptors));


Missing space after exports,.

tniessen · 2017-07-26T23:25:48Z

test/addons-napi/test_dataview/test_dataview.c

+    DECLARE_NAPI_PROPERTY("CreateDataView", CreateDataView)
+  };
+
+  NAPI_CALL_RETURN_VOID(env,napi_define_properties(


Missing space after env,.

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 <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

tniessen · 2017-07-27T00:44:28Z

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

@addaleax surprise! 😛

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 <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

shivanth added 2 commits July 20, 2017 01:55

n-api: Add support for DataView

43ec67d

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

Fix typos in documentation

b56c76b

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

Fix more documentation issue

3839165

addaleax reviewed Jul 20, 2017

View reviewed changes

shivanth added 2 commits July 20, 2017 22:12

Fix typos and alignment

5fc5ffb

Add indentation and fix alignment

260ad4e

cjihrig reviewed Jul 20, 2017

View reviewed changes

doc/api/n-api.md Outdated

### *napi_is_dataview*

<!-- YAML

added: v8.0.0

Copy link

Contributor

cjihrig Jul 20, 2017

Choose a reason for hiding this comment

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

REPLACEME here too please.

cjihrig reviewed Jul 20, 2017

View reviewed changes

shivanth added 2 commits July 21, 2017 01:22

Changes as per review

11362b5

Add links below

b5fcb56

addaleax approved these changes Jul 20, 2017

View reviewed changes

cjihrig approved these changes Jul 21, 2017

View reviewed changes

Relocate the links in doc

3fdc71e

TimothyGu reviewed Jul 21, 2017

View reviewed changes

Changes to intendation and variable names

dcac0ac

TimothyGu reviewed Jul 21, 2017

View reviewed changes

Changes as per review

afca265

TimothyGu approved these changes Jul 21, 2017

View reviewed changes

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

tniessen requested changes Jul 26, 2017

View reviewed changes

tniessen closed this Jul 27, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

digitalinfinity mentioned this pull request Aug 31, 2017

APIs for ES6 DataView nodejs/abi-stable-node#202

Closed

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged


		Returns `napi_ok` if the API succeeded.

		This API checks if the Object passsed in is a dataview.



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

		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.

N api dataview #14382

N api dataview #14382

Conversation

shivanth commented Jul 20, 2017 • edited by vsemozhetbyt

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Jul 20, 2017 • edited

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Jul 21, 2017 • edited

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jul 21, 2017

addaleax commented Jul 26, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen Jul 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Jul 27, 2017

shivanth commented Jul 20, 2017 •

edited by vsemozhetbyt

vsemozhetbyt commented Jul 20, 2017 •

edited

addaleax Jul 21, 2017 •

edited

tniessen Jul 26, 2017 •

edited