Add emplacement for cxx::UniquePtr.
#27
Conversation
| let uninit = std::mem::transmute(uninit_ptr.as_mut().unwrap()); | ||
| let result = n.try_new(uninit); | ||
| if let Err(err) = result { | ||
| T::free_uninitialized_cpp_storage(uninit_ptr); |
There was a problem hiding this comment.
Note that this is not panic safe and will leak memory on panic. Most of the library isn't either but a // FIXME: Not panic safe might be appropriate.
There was a problem hiding this comment.
I tried to do better here using panic::catch_unwind, but we would need to constrain N to be UnwindSafe within the trait definitions of Emplace/EmplaceUnpinned so I gave up for now. Let me know if you think that would be good or not?
There was a problem hiding this comment.
I'm ok with dealing with it later. The right way to do this is actually to have a type that captures the uninit_ptr and frees it in its destructor, which you forget() at the end of the scope. Unwinding out of this frame will run the destructor.
I need to go through the whole crate and add various panic guards so I wouldn't worry about it right now.
src/cxx_support.rs
Outdated
| T::free_uninitialized_cpp_storage(uninit_ptr); | ||
| return Err(err); | ||
| } | ||
| // TODO - is it possible to remove the need for Emplace to return a pinned |
There was a problem hiding this comment.
I think that, in general, we can't do this. T::emplace is analogous to Box::pin for API purposes... the real issue is that UniquePtr should just provide a function for unpinning itself.
There was a problem hiding this comment.
I had a crack at this before seeing your comment, and added a commit which attempts to make this work.
Logic in my head is:
Pin<Box<T>>should be the thing which implements a fundamental emplace-y trait. Fundamentally,Box<T>is not safe to emplace into; onlyPin<Box<T>>is safe to emplace into.- For convenience,
Box<T>should actually implement some other trait, which calls through toPin<Box<T>>::emplace, so that callers can sayBox::emplace_pinned(...)or similar.
That's the idea. In the commit I pushed, emplace_pinned is also called emplace which gets confusing, but this was to avoid breaking compatibility more than necessary. LMK your thoughts.
src/new/mod.rs
Outdated
| /// This enables an `emplace()` method for [`Box`], [`Rc`], and [`Arc`]. Users | ||
| /// are encouraged to implement this function for their own heap-allocated smart | ||
| /// pointers. |
There was a problem hiding this comment.
This comment needs to be moved, I think, plus a clarification on why the trait has been split.
| let uninit = std::mem::transmute(uninit_ptr.as_mut().unwrap()); | ||
| let result = n.try_new(uninit); | ||
| if let Err(err) = result { | ||
| T::free_uninitialized_cpp_storage(uninit_ptr); |
There was a problem hiding this comment.
I'm ok with dealing with it later. The right way to do this is actually to have a type that captures the uninit_ptr and frees it in its destructor, which you forget() at the end of the scope. Unwinding out of this frame will run the destructor.
I need to go through the whole crate and add various panic guards so I wouldn't worry about it right now.
This allows cxx::UniquePtr::emplace of any New made using
moveit norms.
UniquePtr is different from the other smart pointers supported
by moveit, in that UniquePtr is _intrinsically_ pinned. You can't
get an unpinned mutable reference to its contents; therefore there
is no need to use Pin<UniquePtr>. This commit therefore splits
the Emplace trait into Emplace and EmplaceUnpinned, with the goal
that users can naturally call {Box, Arc, Rc, UniquePtr}::emplace
in a similar fashion, so long as both traits are in scope.
UniquePtr<T> emplacement support requires the T to implement
a new trait, MakeCppStorage, which can provide uninitialized
space for the T in some heap cell which can later be deleted
using the std::unique_ptr default destructor. It's not expected
that most users will implement this trait, but that it will
instead be generated automatically by code generators atop moveit
and cxx (for instance, autocxx).
Testing these cxx bindings requires inclusion of C++ code,
and we don't want to link against C++ code for the production
rlib - therefore a separate module is created within the workspace
to host these tests.
|
Duly rebased into a single descriptive commit (and your two remaining nits addressed). Thanks! |
|
Are you waiting for anything from me here @mcy? I have an |
Users of
cxxmay well want toemplaceinto cxx'sUniquePtr. This PR adds such support.There are a number of issues yet to be resolved here. Specifically:
The contract of
emplacealways returns aPin<SmartPtr<T>>,whereas we want to return a simple
UniquePtr<T>, becauseUniquePtris always intrinsically pinned.
Because of this mismatch, there are some nasty transmutes, and the test code is incomplete.
As there appears to be no way to run a build script separately
for tests, we are pulling in some C++ into the production code.
Still, it feels ready for an early look @mcy!
For users of plain
cxx, there's a lot of boilerplate to be written. Of course, the idea is that 100% of the boilerplate here is created byautocxx. For testing purposes, though, this code depends only oncxxnotautocxx.