Skip to content

Add acquire_value API to simplify caller code #944

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 11, 2016
Merged

Add acquire_value API to simplify caller code #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jerry-core/jerry-api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ jerry_api_value_t jerry_api_create_string_value (jerry_api_string_t *value);
jerry_api_size_t jerry_api_string_to_char_buffer (const jerry_api_string_t *, jerry_api_char_t *, jerry_api_size_t);
jerry_api_string_t *jerry_api_acquire_string (jerry_api_string_t *);
jerry_api_object_t *jerry_api_acquire_object (jerry_api_object_t *);
jerry_api_value_t *jerry_api_acquire_value (jerry_api_value_t *);

void jerry_api_release_object (jerry_api_object_t *);
void jerry_api_release_string (jerry_api_string_t *);
Expand Down
33 changes: 33 additions & 0 deletions jerry-core/jerry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,41 @@ jerry_api_release_object (jerry_api_object_t *object_p) /**< pointer acquired th
ecma_deref_object (object_p);
} /* jerry_api_release_object */

/**
* Acquire specified Jerry API value.
*
* Note:
* For values of string and object types this acquires the underlying data,
* for all other types it is a no-op.
*
* Warning:
* Acquired pointer should be released with jerry_api_release_value
*
* @return pointer that may be used outside of the engine
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing @return ... comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

jerry_api_value_t *
jerry_api_acquire_value (jerry_api_value_t *value_p) /**< API value */
{
jerry_assert_api_available ();

if (value_p->type == JERRY_API_DATA_TYPE_STRING)
{
jerry_api_acquire_string (value_p->u.v_string);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing return statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duh. Sorry for the silly mistake. Added the return statement at the bottom of the function.

}
else if (value_p->type == JERRY_API_DATA_TYPE_OBJECT)
{
jerry_api_acquire_object (value_p->u.v_object);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

}
Copy link
Member

Choose a reason for hiding this comment

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

The type of a jerry API value (given with the jerry_api_data_type_t enum) can be different from string or object. The function does not deal with them (there is no final else branch) nor does it document what the expected outcome on such a value is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akiss77 In my opinion, this is an implementation detail. Perhaps in the future, new data types have acquire/release semantics as well, in which case jerry_api_acquire_value and jerry_api_release_value should do the right thing. Note that jerry_api_release_value does not have an else clause either.

Copy link
Member

Choose a reason for hiding this comment

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

I humbly disagree. Internal-only functions can be vague about implementation details (even though it's not ideal but that's how it works in practice) but this is the public API of jerry. It may be a no-op for everything beyond strings and objects but it must document this. (My comment above also mentioned "nor does it document".)

And true enough, jerry_api_release_value has a deficiency in its documentation. This is no justification for making the same mistake in a new function. Better to get the new function right and fix the existing doc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough! I added documentation to both.


return value_p;
} /* jerry_api_acquire_value */

/**
* Release specified Jerry API value
*
* Note:
* For values of string and object types this releases the underlying data,
* for all other types it is a no-op.
*/
void
jerry_api_release_value (jerry_api_value_t *value_p) /**< API value */
Expand Down