Support automatic heap-copies of temporary objects

[tim-janik]

Hi @pmed.

After more than 2 years, I'm still dragging along an important v8pp patch for heap-allocated temporary objects in my fork (in fact it's the only reason I still need to keep my own fork).

See, I have a large number of auto generated C++ functions (from IDL), some of which return temporary objects, and a large number of corresponding auto generated .set ("accessor", &ObjectType::accessor) method registrations.

I'd really like to find a way to upstream the possibility of automatically copying return values that are temporary objects onto the heap, so they can be passed on to V8. My patch essentially adds a one-liner to convert<T>::to_v8(), that does import_external (new Class (value)). Since you didn't seem to like to take the patch in https://github.com/pmed/v8pp/issues/39#issuecomment-282161979, I'm trying to find another way:

1) I need to avoid changing all my auto-generated C++ functions (or to manually sort them apart for ones that return a temporary class and those that don't).

2) I need to avoid changing hand-picked .set ("accessor", &ObjectType::accessor) statements. I could change all of them easily though, if that helped...

So I'd like to have your feedback on what could work:

• Would a variant of .set() be acceptable, e.g. .xset ("accessor", &ObjectType::accessor) where xset() works exactly like set() for all types but additionally copies temporary return values instead of just throwing "failed to wrap C++ object"?

• Or, would my patch be acceptable, if convert::to_v8() is special cased, so it only creates a heap copy if std::is_copy_constructible<T>::value or if std::is_move_constructible<T>::value?

• Or, since you mentioned in #39 that you wanted some explicit user involvement to enable the copies, would a define like V8PP_AUTO_COPY_TEMPORARIES (defaulting to false) help to accept and activate the patch?

• Are there any other ways I'm missing?

Here's the latest version of my patch (against v1.6.0): https://github.com/tim-janik/v8pp/commit/f2720534bdf95a691a7ec290996434359c88f877

Here's the original patch (against v1.4.1): https://github.com/tim-janik/v8pp/commit/cda26b60e3b070ead3dca776ec5699f11b0d52a1

PS: FWIW, I'd consider #39 essentially answered and closable. This issue in contrast is about actually merging functionality that seems to be useful to several downstream projects.

[pmed]

Hi @tim-janik

I've added v8pp::class_::auto_wrap_objects() function to set a flag, that allows automatic wrapping of C++ objects returned from a C++ function. Actual copy and wrapping is performed in a class_::find_object(obj) function overloading for a C++ referenced to obj.

Please see the test_auto_wrap_objects() for a usage example. I would appreciate for a feedback, whether this feature is usable for you.

This implementation has also been backported from master to the recent c++17 branch. The issue is still open, because I also need to update the documentation.

Best regards, Pavel

[tim-janik]

Thanks @pmed.

One question, I was expecting auto_wrapobjects() to instantiate a template that calls the copy ctor. So that having a copy ctor only becomes a requirement for class<> instances that call auto_wrapobjects() and not for others. Wouldn't the current implementation cause compiler errors when class<> wraps a non-copyable type (e.g. a singleton) without calling auto_wrap_objects() ?

As an aside, I've run into major stability problems with my v8pp-based electron module (likely due to Electron instead of v8pp). But since debugging in Electron is exceptionally hard, we're probably moving to a JSON+IPC solution instead of using a v8pp based module. Our Jsonipc binding has taken lots of inspiration from the v8pp API, thank you for a great design. Here's the function implementing automatic copying/wrapping behavior via template instantiation as suggested above: https://github.com/tim-janik/beast/blob/a48588c8d838fc008c2c0651aa67b9f43c5c1162/jsonipc/jsonipc.hh#L891