Skip to content

Allow polymorphism with fine::ResourcePtr #8

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow polymorphism with fine::ResourcePtr #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d07346a
Allow polymorphism with `fine::ResourcePtr`
brodeuralexis Jun 10, 2025
ba563ff
Add name to nameless struct and union members
brodeuralexis Jun 10, 2025
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
26 changes: 26 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,32 @@ class Generator {
If defined, the `destructor` callback is called first, and then the
`T` destructor is called as usual.

To make use of polymorphism with resources, only the base class must be
declared using `FINE_RESOURCE`. To construct derived classes, use the
`fine::make_resource<Base, Derived>(...)` function:

```c++
class Supplier {
public:
virtual ~Supplier() noexcept = default;

virtual std::int64_t supply() = 0;
};
FINE_RESOURCE(Supplier);

class ConstantSupplier final : public Supplier {
public:
ConstantSupplier(std::int64_t constant) : m_constant(constant) {}

std::int64_t supply() { return m_constant; }

private:
std::int64_t m_constant;
};

fine::ResourcePtr<Supplier> supplier = fine::make_resource<Supplier, ConstantSupplier>(INT64_C(42));
```

Oftentimes NIFs deal with classes from third-party packages, in which
case, you may not control how the objects are created and you cannot
add callbacks such as `destructor` to the implementation. If you run
Expand Down
64 changes: 45 additions & 19 deletions include/fine.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,36 @@ template <typename... Args> class Error {

namespace __private__ {
template <typename T> struct ResourceWrapper {
T resource;
bool initialized;
union {
struct {
bool initialized;
} data;
std::max_align_t _unused;
} payload;

bool initialized() const { return payload.data.initialized; }

void set_initialized(bool initialized) {
payload.data.initialized = initialized;
}

const T *resource() const { return reinterpret_cast<const T *>(this + 1); }

T *resource() noexcept { return reinterpret_cast<T *>(this + 1); }

template <typename U, typename = std::enable_if_t<std::is_base_of_v<T, U>>>
static constexpr std::size_t byte_size() noexcept {
return sizeof(ResourceWrapper) + sizeof(U);
}

static void dtor(ErlNifEnv *env, void *ptr) {
auto resource_wrapper = reinterpret_cast<ResourceWrapper<T> *>(ptr);

if (resource_wrapper->initialized) {
if (resource_wrapper->initialized()) {
if constexpr (has_destructor<T>::value) {
resource_wrapper->resource.destructor(env);
resource_wrapper->resource()->destructor(env);
}
resource_wrapper->resource.~T();
resource_wrapper->resource()->~T();
}
}

Expand Down Expand Up @@ -221,11 +240,11 @@ template <typename T> class ResourcePtr {
return *this;
}

T &operator*() const { return this->ptr->resource; }
T &operator*() const { return *this->ptr->resource(); }

T *operator->() const { return &this->ptr->resource; }
T *operator->() const { return this->ptr->resource(); }

T *get() const { return &this->ptr->resource; }
T *get() const { return this->ptr->resource(); }

friend void swap(ResourcePtr<T> &left, ResourcePtr<T> &right) {
using std::swap;
Expand All @@ -241,12 +260,17 @@ template <typename T> class ResourcePtr {
// Friend functions that use the resource_type static member or the
// private constructor.

template <typename U, typename... Args>
template <typename U, typename V, typename... Args, typename>
friend ResourcePtr<U> make_resource(Args &&...args);

template <typename U>
friend Term make_resource_binary(ErlNifEnv *env, ResourcePtr<U> resource,
const char *data, size_t size);

friend class Registration;

friend struct Decoder<ResourcePtr<T>>;
friend struct Encoder<ResourcePtr<T>>;

inline static ErlNifResourceType *resource_type = nullptr;

Expand All @@ -255,7 +279,8 @@ template <typename T> class ResourcePtr {

// Allocates a new resource object, invoking its constructor with the
// given arguments.
template <typename T, typename... Args>
template <typename T, typename U = T, typename... Args,
typename = std::enable_if_t<std::is_base_of_v<T, U>>>
ResourcePtr<T> make_resource(Args &&...args) {
Comment on lines +282 to 284
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have a make_polymorphic_resource<Base, Concrete>(...) function instead, and have make_resource<T>(...) as a alias for make_polymorphic_resource<T, T>(...) ?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @brodeuralexis!

I think if we want to make it polymorphic, ideally we should keep it close the the std smart pointers (and regular pointers), so similarly to:

std::shared_ptr<Base> ptr = std::make_shared<Derived>();

we would have:

fine::ResourcePtr<AbstractRes> res = fine::make_resource<ConcreteRes>();

My understanding is that we would need a constructor with std::is_convertible_v<U*, T*> check. The issue is that fine::ResourcePtr<T> does not store T* directly, it stores fine::ResourceWrapper<T>*.

Perhaps we could kill ResourceWrapper if we manage the memory alignment without that struct. So fine::ResourcePtr<T> would store T*, then when allocating the resource we would allocate memory space for both initialized flag and T.

I was trying to avoid messing with alignment manually, but you do it in the PR also, so perhaps there is no way to avoid it either way.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Gah, I guess we need to know AbstractRes upfront, because it's the one with FINE_RESOURCE.

Copy link
Member

@jonatanklosko jonatanklosko Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, we allow for defining optional callbacks on the resource class, currently we only have one: void destructor(ErlNifEnv *env).

So if they define it on ConcreteRes and do not define as virtual method on AbstractRes, it wouldn't be called. So it could also be a footgun.

Do you have a use case where you actually needed a polymorphic resource like this? For example, I think you could do this:

struct MyResource {
  std::unique_ptr<Abstract> my_object;
}
FINE_RESOURCE(MyResource);

This way we move the polymorphism one level down.

I actually think it's good to keep the resource structs simple.

Copy link
Contributor Author

@brodeuralexis brodeuralexis Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My goal with this feature was to remove the need for an allocation using std::unique_ptr.

So if they define it on ConcreteRes and do not define as virtual method on AbstractRes, it wouldn't be called. So it could also be a footgun.

We could also argue that the same footgun will happen if ~Abstract is not virtual.

We could introduce fine::Resource with a virtual destructor(ErlNifEnv*) method, and require fine::Registration::register_resource for T where std::is_base_of_v<Resource, T>. This would remove the need for has_destructor, since now all resources will have the destructor method and provide a place to add callbacks if needed with potential default behaviour.

Although this will prevent registering NIF resources on classes the NIF code doesn't control, we could introduce a separate register_resource to deal with such a case.

I was trying to avoid messing with alignment manually, but you do it in the PR also, so perhaps there is no way to avoid it either way.

enif_alloc ensures that memory is aligned for alignof(max_align_t), but I must also ensure that T starts correctly aligned at alignof(max_align_t) even if alignof(T) <= alignof(max_align_t). At the end, the current approach minimizes the use of pointer arithmetic at the cost of using a union.

I actually think it's good to keep the resource structs simple.

This is a feature I believe will make writing more convenient, as it would encourage NIF resources to be more than data containers. I would also be more in line with shared_ptr.

I think if we want to make it polymorphic, ideally we should keep it close the the std smart pointers (and regular pointers) [...]

I stumbled upon the, I need the base resource type for the ErlNifResourceType*`, problem too. I would have to do some tests to see if there is a way to recursively traverse base classes using template metaprogramming.

Otherwise, we could introduce fine::ResourcePtr<T>::make_polymorphic<U, Args...>(Args&&...), and have fine::make_resource<T, Args..>(Args&&...) stay the same. This would make it even more explicit (and more of an advanced API), as well as make the base and concrete templated types more distinguishable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside, I have the feeling that nifpp was also trying to get this to work:
https://github.com/saleyn/nifpp/blob/main/nifpp.h#L747-L751

But while they have the dynamic cast working, their resource creation is not polymorphic.

Copy link
Member

Choose a reason for hiding this comment

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

We could introduce fine::Resource with a virtual destructor(ErlNifEnv*) method, and require fine::Registration::register_resource for T where std::is_base_of_v<Resource, T>.

I did that initially, but I ended up no liking it, because it limits resources only to user-defined classes. And yes, we could have both, but I really think we should have only one abstraction.

I would have to do some tests to see if there is a way to recursively traverse base classes using template metaprogramming.

It's likely doable, though the problem is that someone may also use a derived class as a resource (they may not even know it's derived) and in that case we don't want to find the base one.

I don't like the divergence from std pointers, but yeah, passing both classes explicitly may be the only reasonable way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've had no luck with trying to get base classes through templates without some kind of reflection.

If we include this, I would go for a fine::ResourcePtr<Base>::make_polymorphic<Concrete, Args...>(Args&&...) function. This makes the base and concrete type easily identifiable. fine::make_resource<T, Args...>(Args&&...)'s behaviour would be left unchanged.

This would obviously be a pretty advanced/complex feature for fine. While I believe its inclusion to be worthwhile, I will defer to your opinion, since, as you correctly pointed out, a layer of indirection can always be added through a std::unique_ptr inside the resource to solve the problem at the cost of an extra allocation.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I would rather not expand the API for this purpose, so for now I would not do it. Thank you for discussing this through!

auto type = ResourcePtr<T>::resource_type;

Expand All @@ -265,10 +290,10 @@ ResourcePtr<T> make_resource(Args &&...args) {
" to register your resource type with the FINE_RESOURCE macro");
}

void *allocation_ptr =
enif_alloc_resource(type, sizeof(__private__::ResourceWrapper<T>));
void *allocation_ptr = enif_alloc_resource(
type, __private__::ResourceWrapper<T>::template byte_size<U>());

auto resource_wrapper =
auto *resource_wrapper =
reinterpret_cast<__private__::ResourceWrapper<T> *>(allocation_ptr);

// We create ResourcePtr right away, to make sure the resource is
Expand All @@ -278,13 +303,14 @@ ResourcePtr<T> make_resource(Args &&...args) {
// We use a wrapper struct with an extra field to track if the
// resource has actually been initialized. This way if the constructor
// below throws, we can skip the destructor calls in the Erlang dtor
resource_wrapper->initialized = false;
resource_wrapper->set_initialized(false);

// Invoke the constructor with prefect forwarding to initialize the
// object at the VM-allocated memory
new (&resource_wrapper->resource) T(std::forward<Args>(args)...);
new (reinterpret_cast<U *>(resource_wrapper->resource()))
U(std::forward<Args>(args)...);

resource_wrapper->initialized = true;
resource_wrapper->set_initialized(true);

return resource;
}
Expand All @@ -296,8 +322,8 @@ ResourcePtr<T> make_resource(Args &&...args) {
template <typename T>
Term make_resource_binary(ErlNifEnv *env, ResourcePtr<T> resource,
const char *data, size_t size) {
return enif_make_resource_binary(
env, reinterpret_cast<void *>(resource.get()), data, size);
return enif_make_resource_binary(env, reinterpret_cast<void *>(resource.ptr),
data, size);
}

// Creates a binary term copying data from the given buffer.
Expand Down Expand Up @@ -811,7 +837,7 @@ template <typename K, typename V> struct Encoder<std::map<K, V>> {

template <typename T> struct Encoder<ResourcePtr<T>> {
static ERL_NIF_TERM encode(ErlNifEnv *env, const ResourcePtr<T> &resource) {
return enif_make_resource(env, reinterpret_cast<void *>(resource.get()));
return enif_make_resource(env, reinterpret_cast<void *>(resource.ptr));
}
};

Expand Down
29 changes: 29 additions & 0 deletions test/c_src/finest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,30 @@ struct ExError {
static constexpr auto is_exception = true;
};

struct AbstractRes {
virtual ~AbstractRes() noexcept = default;
};
FINE_RESOURCE(AbstractRes);

struct ConcreteRes : AbstractRes {
ErlNifPid pid;

ConcreteRes(ErlNifPid pid) : pid(pid) {}

~ConcreteRes() noexcept override {
auto target_pid = this->pid;

auto thread = std::thread([target_pid] {
auto msg_env = enif_alloc_env();
auto msg = fine::encode(msg_env, atoms::destructor_default);
enif_send(NULL, &target_pid, msg_env, msg);
enif_free_env(msg_env);
});

thread.detach();
}
};

int64_t add(ErlNifEnv *, int64_t x, int64_t y) { return x + y; }
FINE_NIF(add, 0);

Expand Down Expand Up @@ -196,6 +220,11 @@ fine::Term resource_binary(ErlNifEnv *env,
}
FINE_NIF(resource_binary, 0);

fine::ResourcePtr<AbstractRes> resource_abstract(ErlNifEnv *, ErlNifPid pid) {
return fine::make_resource<AbstractRes, ConcreteRes>(pid);
}
FINE_NIF(resource_abstract, 0);

fine::Term make_new_binary(ErlNifEnv *env) {
const char *buffer = "hello world";
size_t size = 11;
Expand Down
1 change: 1 addition & 0 deletions test/lib/finest/nif.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ defmodule Finest.NIF do
def resource_create(_pid), do: err!()
def resource_get(_resource), do: err!()
def resource_binary(_resource), do: err!()
def resource_abstract(_pid), do: err!()

def make_new_binary(), do: err!()

Expand Down
7 changes: 7 additions & 0 deletions test/test/finest_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ defmodule FinestTest do

assert_receive :destructor_default
end

test "resource can be abstract" do
NIF.resource_abstract(self())
:erlang.garbage_collect(self())

assert_receive :destructor_default
end
end

describe "make_new_binary" do
Expand Down