Skip to content
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

[embind] Add return value policy option for function bindings. #21692

Merged
merged 7 commits into from
May 13, 2024

Conversation

brendandahl
Copy link
Collaborator

@brendandahl brendandahl commented Apr 4, 2024

This enables C++ function bindings to explicitly control what happens
to an object when returned to JS via BindingType::toWireType. The new
polices are: default(no policy), take_ownership, and reference. The
polices are further explained in the updated documentation.

To support the polices BindingType::toWireType now uses tag dispatching
to allow specialization for each of the return policies. Custom
BindingTypes that do not want to specialize each option can use the
return_value_policy::default_tag which will match all the policies.

Note: this is the framework for further work on improving class property
bindings and allowing more idiomatic JS.

This enables C++ function bindings to explicitly control what happens
to an object when returned to JS via BindingType::toWireType. The new
polices are: default(no policy), take_ownership, and reference. The
polices are further explained in the updated documentation.

To support the polices BindingType::toWireType now uses tag dispatching
to allow specialization for each of the return policies. Custom
BindingTypes that do not want to specialize each option can use the
return_value_policy::default_tag which will match all the policies.

Note: this is the framework for further work on improving class property
bindings and allowing more idiomatic JS.
on the return type of the function. The policies work as follows:

* default (no argument) - Use the objects copy constructor since this is generally
safe and decouples returned values lifetime from the original.
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand what this means. We use the copy constructor, but then what happens to the object? There is no C++ code for it to live in, so it seems like RAII would immediately clean it up..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The copy is a new allocation, so JS gets a pointer and is then responsible for deleting the object. The value/reference that the new object was created from will be automatically deleted by C++.

I'll reword this comment and add some more detail.


* *default (no argument)* - For return by value and reference a new object will be allocated using the
object's copy constructor. JS then owns the object and is responsible for deleting it. Returning a
pointer is not allowed by default (use an explicit policy below).
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, even with the new text I'm confused here. This is probably my own fault 😄 But say we have a function that returns by value,

Object getObject() { .. }

What is, effectively, the bindings code for that? The text here says the copy constructor is called, so I am imagining

void* bind_getObject() {
  Object copy = getObject(); // copy constructor occurs here during the `=` operation
  ...
}

And somehow in the ... we transfer ownership to JS. But how does copy not end up collected by C++ when that scope ends?

To put my question another way, how can you call the copy constructor without opting into automatic memory management?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The copy constructor is explicitly called with a new. e.g. the code for the above would be:

void* bind_getObject() {
  return new Object(getObject());
}

Or in terms of the embind, that happens in the toWireType function here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks, so it's a raw new that uses the copy constructor. Makes sense to me now.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

The approach, docs and tests lgtm. I can't say I can easily follow the C++ template magic, but it looks generally ok to me.

cc @RReverser who may be interested in this.

@@ -2843,7 +2843,7 @@ EMSCRIPTEN_BINDINGS(noncopyable) {
.function("method", &Noncopyable::method)
;

function("getNoncopyable", &getNoncopyable);
function("getNoncopyable", &getNoncopyable, return_value_policy::take_ownership());
Copy link
Member

Choose a reason for hiding this comment

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

To make sure I understand, this is a necessary change because this is non-copyable, and the default return value policy would copy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missed this comment. Yes that's correct. This will be a breaking change, though at least the compiler will catch it and I think it's pretty uncommon.

@brendandahl brendandahl merged commit 799a1cb into emscripten-core:main May 13, 2024
29 checks passed
mcdurdin added a commit to keymanapp/keyman that referenced this pull request Aug 21, 2024
Emscripten 3.1.60 made a breaking change to `BindingType::toWireType`
(why is this done in a patch version?) so this change ensures that we
can continue to build on 3.1.58 and 3.1.64 (which is needed for #12234),
as a bridging strategy. We will need to merge this into 17.0-stable as
well so that we can upgrade the build agents to 3.1.64.

Relates-to: emscripten-core/emscripten#21692
Relates-to: #12234
mcdurdin added a commit to keymanapp/keyman that referenced this pull request Aug 21, 2024
Emscripten 3.1.60 made a breaking change to `BindingType::toWireType`
(why is this done in a patch version?) so this change ensures that we
can continue to build on 3.1.58 and 3.1.64 (which is needed for #12234),
as a bridging strategy. We will need to merge this into 17.0-stable as
well so that we can upgrade the build agents to 3.1.64.

Relates-to: emscripten-core/emscripten#21692
Relates-to: #12234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants