search

bytecodealliance / wasmtime-cpp

Apache License 2.0
85 stars 18 forks source link

Add Store::Context::set_data() and Store::Context::get_data() #15

Closed dshiell15 closed 3 years ago

dshiell15 commented 3 years ago

also make the Trap message() method const. Cheers!

alexcrichton commented 3 years ago

Thanks!

I don't have a ton of experience binding C apis in C++, but do you know if it's common to expose void* like this? For example we could otherwise also manage it with something like std::any automatically, but I don't know if it's idiomatic to add a layer like that.

dshiell15 commented 3 years ago

Good question, I feel like std::any would probably be the more c++ idiomatic way. Let me look into that.

dshiell15 commented 3 years ago

Hmmm, current broken-ness aside...

As I started writing a few more tests and changing the implementation to use std::any I ran into an issue/maybe a bug? It looks like the store can take a finalizer for the data when it is created which is called when the store destructs. However, if I implement the finalizer (similar to ExternRef) and call set_data() with a new object it looks like the finalizer is not called which results in a memory leak (for example, if I wrap the std::any in a unique pointer and release it).

Alternatively I could make the caller wrap the data in a std::any prior to calling, but this doesn't seem very user friendly. Maybe the finalizer just needs to be invoked whenever wasmtime_context_set_data() is called?

Any thoughts @alexcrichton? I'm happy to go with whatever you think is best.

dshiell15 commented 3 years ago

Maybe a better solution would be to always retrieve any existing data and clean it up before setting new data. This would be pretty straight forward.

alexcrichton commented 3 years ago

Nah yeah I think the C API can be improved in this regard. One improvement could perhaps be to specify the finalizer and data on set_data, and that way it's clear that it can always be a uniquely owned value and any previous value is destroyed.

dshiell15 commented 3 years ago

Yeah that would be nice. I think right now the data finalizer can only be set during store creation?

After my latest update, data can either be copied/moved in during the conversion to std::any. In both cases, I think the finalizer should be run so perhaps this is workable?

alexcrichton commented 3 years ago

Ok seems reasonable to me to go ahead and land this, thanks!