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

Idiomatic smartpointer generation #141

Open
kyonifer opened this issue May 10, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@kyonifer
Copy link

commented May 10, 2019

Suppose I want to wrap

struct A {};
void foo(std::shared_ptr<A> p){ }

If I wrap with

mod.add_type<A>("A").constructor();
mod.method("foo", &foo);

Then trying to generate an A in Julia and call the function fails with

julia> a=A()
AAllocated(Ptr{Nothing} @0x00007fafde6f2150)

julia> foo(a)
ERROR: MethodError: no method matching foo(::AAllocated)
Closest candidates are:
  foo(::CxxWrap.SmartPointer{T2} where T2<:A) at none:0
Stacktrace:
 [1] top-level scope at none:0

One workaround I found is

a_ptr=CxxWrap.SmartPointerWithDeref{A, :NSt__110shared_ptrIiEE}(a.cpp_object)
foo(a_ptr)

which, if I'm reading the convert chains correctly, will do some work to convert a pointer into a shared_ptr if the quoted mangled name is something with a known conversion. However it's not clear where to harvest the mangled symbol names from other than the compiled library, which is ugly. Of course, one could also write shims in C++ that return smart pointers.

In pybind11 they allow specification of a holder type which allows python to wrangle python-owned references into a selected smart pointer type. However, add_type<A, std::shared_ptr<A>> doesn't seem to work. I'm wondering if there is anything equivalent in CxxWrap, or maybe a convenience method somewhere that I didn't find?

@barche

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

If A is consistently passed as a shared_ptr, it is best to immediately make its Julia constructor return one:

namespace jlcxx
{
template<> struct DefaultConstructible<A> : std::false_type {};
}

mod.add_type<A>("A");
mod.method("A", [] () { return std::make_shared<A>(); });
mod.method("foo", &foo);

The DefaultConstructible specialization is needed because otherwise CxxWrap will automatically generate the constructor (the .constructor() in your code was redundant). Objects returned by these "automatic" CxxWrap constructors automatically have a finalizer that deletes the allocated object, so they can't safely be placed in a shared_ptr because a double free would occur.

@kyonifer

This comment has been minimized.

Copy link
Author

commented May 11, 2019

That approach does indeed work for my toy example. Unfortunately the library I'm trying to write bindings for passes objects around quite a lot. Objects can be returned by-value or as a shared_ptr depending on if shared ownership semantics are required or not, and it's rather heterogeneous.

I'd prefer not to cripple the julia bindings by locking them into a particular pointer type, because I would prefer to mostly work with this library in Julia.

One answer is to ask the user to manually wrap things into/out of smart pointers themselves. For example, if the user needs to take the result of A findFoo() and pass it back into void foo(std::shared_ptr<A> p){ } I could expose a function make_shared to Julia such that the user could call foo(makeShared(findFoo())), but that is an awkward solution as we'd have to intercept all by-value returns that get sent to Julia to make sure they don't end up in the automatic container with a finalizer that nukes them when julia loses all references (thus putting them into a shared_ptr would be unsafe, as you noted).

@barche

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

It should always be OK to return a shared pointer from the constructor, conversion from shared pointer to value is done automatically for functions that take values or references as argument.

For the findFoo example, the make_shared approach can work, but make_shared should return a shared pointer to a copy of the input (e.g. std::make_shared<A>(a)). Alternatively you can wrap findFoo in a lambda that directly returns a shared pointer, it really depends on which use case happens most often. Even in pure C++, having a value and needing to convert it to a shared pointer is a pain, so I would guess this should be the rarest use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.