Skip to content

jerry: finalize hint for array buffers#421

Merged
yorkie merged 1 commit intomasterfrom
jerry/arraybuffer
Nov 28, 2018
Merged

jerry: finalize hint for array buffers#421
yorkie merged 1 commit intomasterfrom
jerry/arraybuffer

Conversation

@legendecas
Copy link
Copy Markdown
Contributor

@legendecas legendecas commented Nov 23, 2018

  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added

As ArrayBuffer's native pointer shall be a writable buffer address, an additional finalize hint is required to determine the informations of the native pointer on finalization.

Copy link
Copy Markdown
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

Leaves some questions :)

sum += data[i];
}

jerry_release_value (buffer);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why removing this release operation to buffer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned in PR description, it's not that comprehensive to acquire the object on we access it's info. While the side effects on accessing info has been removed, the additional release operation should be deleted.

return (uint8_t *const) mem_buffer_p;
}
lit_utf8_byte_t *mem_buffer_p = ecma_arraybuffer_get_buffer (buffer_p);
return (uint8_t *const) mem_buffer_p;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the change for? We still need check external and acquire the object as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If ArraryBuffer is not an external buffer, a null pointer is returned before. This is not acceptable if we do expect data been transferred from JS land.

function calls. In any other case this function will return `NULL`.

After using the pointer the [jerry_release_value](#jerry_release_value)
function must be called.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if the buffer is external, user should use jerry_release_value manually to manage the heap objects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the ArrayBuffer object it self is managed by Jerry's GC actually.

@yorkie yorkie added the minor minor changes label Nov 28, 2018
Copy link
Copy Markdown
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

LGTM

@yorkie yorkie merged commit 00d77aa into master Nov 28, 2018
@yorkie yorkie deleted the jerry/arraybuffer branch November 28, 2018 05:25
algebrait added a commit that referenced this pull request Nov 28, 2018
qile222 pushed a commit that referenced this pull request Dec 13, 2018
* master: (35 commits)
  https: client request doesn’t define default encoding (#440)
  n-api: data pointer was NULL on getting typed array info (#441)
  os: build bcast address interface. (#439)
  assert: better deepStrictEqual assertion (#435)
  working on v0.11.x (#434)
  process: memory leaks on recursive ticking (#433)
  uv, os: implement os.{get,set}Priority functions (#409)
  jerry: implement ES2015 class feature (part II.) (#428)
  test: fix wrong travis diff target introduced by #425 (#429)
  jerry: pass and enable jerry-test-suite (#425)
  n-api: ArrayBuffer/TypedArray support (#419)
  deps: upgrade the mbedtls to 2.13.0-apache (#384)
  jerry: rework jerry_parse function (#422)
  jerry: finalize hint of array buffers (#421)
  jerry: reduce the argument count of ecma_op_object_get_property_names (#424)
  n-api: update headers/test suites to LTS(10.13.0) (#416)
  jerry: Date.now shall return an integer (#418)
  n-api: thread safe functions (#411)
  util: IOTJS_ASSERT prints stack trace on macOS (#415)
  process: set immediate shall start an idle handle to activate uv_loop (#417)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants