Implement an initial implementation of DataView class #205
Conversation
|
@mhdawson PTAL |
|
FYI, I found a bug in upstream. So, I sent a pull request (nodejs/node#17869). |
|
Gentle ping :) |
|
Just catching up after being away for a few weeks. Will take me a couple more days before I can review. |
| nullptr /* arrayBuffer */, | ||
| &byteOffset /* byteOffset */); | ||
| NAPI_THROW_IF_FAILED(_env, status, 0); | ||
| return byteOffset; |
There was a problem hiding this comment.
Does it make any sense to get the data once and cache it as opposed to requesting it every time, I think at least some of them will not change.
There was a problem hiding this comment.
I considered that but I thought that it would be better to request the data everytime than spending additional memory. I think that ByteOffset(), ByteLength(), and ArrayBuffer() methods are necessary but I'm not sure if they are very often used. So, isn't it better to reduce memory usage? (Also, if users want to cache it, they can do it themselves.) WDYT?
There was a problem hiding this comment.
I'm ok with keeping it simple for now, if we find it makes sense later on we can always add the cache.
napi.h
Outdated
| size_t ByteOffset() const; ///< Gets the offset into the buffer where the array starts. | ||
| size_t ByteLength() const; ///< Gets the length of the array in bytes. | ||
|
|
||
| // TODO: Should implement more methods to read/write data into array buffer. |
There was a problem hiding this comment.
Maybe explain here that this is a first step and support for additional methods is being added incrementally.
|
A few comments. One other general thought is that it would be great to start adding the doc at the same time when we add new APIs. |
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them
There was a problem hiding this comment.
A few comments. One other general thought is that it would be great to start adding the doc at the same time when we add new APIs.
I agree with you totally. But this implementation is still behind macro flag.
Although users can not use this feature yet, it might be confusing if updating the documents. Therefore, I think that updating documents is better when when exposing this API(removing the macro flag). WDYT?
| nullptr /* arrayBuffer */, | ||
| &byteOffset /* byteOffset */); | ||
| NAPI_THROW_IF_FAILED(_env, status, 0); | ||
| return byteOffset; |
There was a problem hiding this comment.
I considered that but I thought that it would be better to request the data everytime than spending additional memory. I think that ByteOffset(), ByteLength(), and ArrayBuffer() methods are necessary but I'm not sure if they are very often used. So, isn't it better to reduce memory usage? (Also, if users want to cache it, they can do it themselves.) WDYT?
|
Makes sense to make sure doc is added before we remove the gating flag. |
|
Gentle ping :) |
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them PR-URL: #205 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>
|
Landed as 673b59d |
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them PR-URL: nodejs/node-addon-api#205 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them PR-URL: nodejs/node-addon-api#205 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them PR-URL: nodejs/node-addon-api#205 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>
This is an initial implementation of DataView class. This change includes the following things: - DataView::New() methods - Getters for DataView (ArrayBuffer(), ByteOffset(), ByteLength()) - Tests for them PR-URL: nodejs/node-addon-api#205 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>
This is an initial implementation of DataView class. This change
includes the following things: