-
Notifications
You must be signed in to change notification settings - Fork 673
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
Add ArrayBuffer detach operations #3208
Add ArrayBuffer detach operations #3208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, please fix the style issues reported by Travis.
53d52c6
to
2b5ac1f
Compare
Style fixed :) But looks like travis failed to install dependencies :( https://travis-ci.org/jerryscript-project/jerryscript/jobs/595415709 |
5984f7d
to
65d48be
Compare
There was a problem hiding this 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 after fixing these minor style issues.
docs/02.API-REFERENCE.md
Outdated
jerry_value_t res = jerry_detach_arraybuffer (buffer); | ||
|
||
// release buffer as it is not needed after this point | ||
jerry_release_value(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space before (
/** | ||
* Helper function: check if the target ArrayBuffer is detached | ||
* | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 new line is enough
/* in case the arraybuffer has been detached */ | ||
return array_p->buffer_p == NULL; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO return false;
is enough here, the else block is unnecessary.
} | ||
else | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
jerry-core/api/jerry.c
Outdated
{ | ||
return jerry_throw (ecma_raise_type_error (ECMA_ERR_MSG ("Expects a detachable ArrayBuffer."))); | ||
} | ||
return ECMA_VALUE_UNDEFINED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to return ECMA_VALUE_TRUE
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked ECMA spec for the return value. It's expecting null. I'll update to be compliant with the spec. Would this be ok with your suggestion?
tests/unit-core/test-arraybuffer.c
Outdated
|
||
jerry_value_t is_detachable = jerry_is_arraybuffer_detachable (arraybuffer); | ||
TEST_ASSERT (!jerry_value_is_error (is_detachable)); | ||
TEST_ASSERT (!jerry_value_to_boolean (is_detachable)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we also should test that the type of the return value is ok. By using to_boolean
we force a conversion if the return value was not a boolean. We can simply use the jerry_get_boolean_value
.
d1aa140
to
e5ebaa0
Compare
JerryScript-DCO-1.0-Signed-off-by: legendecas [email protected]
e5ebaa0
to
6ed1b35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implement 24.1.1.3 DetachArrayBuffer( arrayBuffer ) operation.
JerryScript-DCO-1.0-Signed-off-by: legendecas [email protected]