yet-another-user / pimpl

The C++ Pimpl Idiom Again!
36 stars 7 forks source link

onstack: support moving implementation #7

Closed muggenhor closed 7 years ago

muggenhor commented 7 years ago

This allows implementations to be moved if they support it.

yet-another-user commented 7 years ago

Thank you for your contribution. Truly and much appreciated. I totally agree with you adding "moveable" construct() and assign() to traits::copyable. However, I am not that sure traits::unique needs those too... because "moveability" is provided by the proxy class (impl_ptr) or more precisely by the unique policy class. That is, the implementation itself is not moved in the case of "unique". Pointers to that implementation are moved. Won't you agree?

yet-another-user commented 7 years ago

OK. I actually had to take "moveable" construct() and assign() out of traits::unique because tests were failing to compile... predictably. Unique test has the following methods purposely deleted (to make sure implementation is unique, i.e. never copied):

template<> struct boost::impl_ptr::implementation { implementation (implementation const&) =delete; implementation& operator=(implementation const&) =delete; }

Those methods are deployed (and fail to compile) in, say,

struct detail::traits::unique { void assign(impl_type p, impl_type&& from) const override { p = std::move(from); } };

yet-another-user commented 7 years ago

I suspect these definitions will not work: virtual void assign (impl_type p, impl_type&& from) const { assign(p, from); } virtual impl_type construct (void* vp, impl_type&& from) const { return construct(vp, from); }

They will be calling themselves recursively. Don't you agree?

muggenhor commented 7 years ago

That is, the implementation itself is not moved in the case of "unique". Pointers to that implementation are moved. Won't you agree?

I agree for the traits. The approach that you use for moving in the policies (i.e. swapping) is something I'm not particularly happy about though. I wanted to create a review for that, but I'm not seeing GitHub provide any support for that. Would you prefer to receive this on the Boost ML?

OK. I actually had to take "moveable" construct() and assign() out of traits::unique because tests were failing to compile... predictably

Ah right, unique doesn't imply movable. Good that you have a test for that ;-)

I suspect these definitions will not work

They should work because 'from' is an lvalue reference and will thus call the lvalue versions of these functions instead. This has the result of asserting if neither are implemented, and copying if only the lvalue versions are overloaded.

yet-another-user commented 7 years ago

I suspect these definitions will not work They should work...

Indeed. "from" is an lvalue. Gosh, that new stuff is awfully confusing. Reverted back to your original version. Much appreciated.

yet-another-user commented 7 years ago

The approach that you use for moving in the policies (i.e. swapping) is something I'm not particularly happy about though. Would you prefer to receive this on the Boost ML?

  1. I believe swap() is the standard mechanism (see std::vector el al).
  2. Boost Dev. list is fine... the more discussion the merrier. it's fairly quiet right now though.
muggenhor commented 7 years ago

Indeed. "from" is an lvalue. Gosh, that new stuff is awfully confusing. Reverted back to your original version. Much appreciated.

I find this rule of thumb pretty useful "if it has a name it's an lvalue".

Boost Dev. list is fine... the more discussion the merrier. it's fairly quiet right now though.

Ack, will take it there. As for quietness, I don't know about Australia, but here (Netherlands & Germany) we're just coming out of the summer vacation period. It's still pretty quiet at the office. So hopefully that's the reason for the quiet on the ML.