Skip to content
This repository was archived by the owner on Jul 25, 2019. It is now read-only.

Feature reimpl arraybuffer#92

Open
kenny-y wants to merge 2 commits intointel:masterfrom
kenny-y:feature-reimpl-arraybuffer
Open

Feature reimpl arraybuffer#92
kenny-y wants to merge 2 commits intointel:masterfrom
kenny-y:feature-reimpl-arraybuffer

Conversation

@kenny-y
Copy link
Contributor

@kenny-y kenny-y commented Dec 13, 2016

No description provided.

…ode's buffer.

v8::ArrayBuffer can work on an external buffer without owning it.

Closes intel#90
@kenny-y
Copy link
Contributor Author

kenny-y commented Dec 13, 2016

@tingshao I've updated the impl of ArrayBuffer, there are a few changes, including a revert of your previous PR which make ArrayBuffer be a persistent member variable of NanXXX wrapper class. The reason of the revert is that we sometimes need to update the buffer length, but with your previous PR, there is no way we can update it since it's cached only once.

private:
void CopyFrom(const ArrayBufferHelper& rhs) {
if (this != &rhs) {
data_ = rhs.data_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the buffer copy using assign cause multiple free if the mode is internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes if external_buffer_ is false, then there can be double delete problem, unless user explicitly change one object and set its external_buffer to true

Can there be an automatically approach to avoid this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the const to change the rhs? Seems not compatible with the coding style. :)

Copy link
Contributor Author

@kenny-y kenny-y Dec 13, 2016

Choose a reason for hiding this comment

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

How about we rename the class to ExternalArrayBuffer and make it only work in external buffer mode?

ArrayBuffers with ownership of buffer will be treated in other class, to avoid potential issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

The choice of whether ExternalArrayBuffer or InternalArrayBuffer is made only in impl code right? I'm ok with that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants