[smart_ptr] refactored enable_shared_from_this
I refactored enable_shared_from_this in order to remove the _owner flag. To test it, you need a small and straight-forward addition to detail::weak_count: It needs .empty(), similar to detail::shared_count. The attached version also hides all internal stuff from the public interface, using 'friend' - maybe some compiler would need a workaround for that. If someone objects to this (maybe because it might cause trouble with some compilers) I could leave this part out or fix it later where it breaks. The good thing of this is, that we no longer need _internal_-prefixes. OK to commit? Regards, Daniel
Daniel Frey:
I refactored enable_shared_from_this in order to remove the _owner flag.
To test it, you need a small and straight-forward addition to detail::weak_count: It needs .empty(), similar to detail::shared_count.
Are you sure that you need to test for emptiness instead of expiration? X x; // derives from esft shared_ptr<X> p1( &x, null_deleter() ); p1->shared_from_this(); // OK p1.reset(); shared_ptr<X> p2( &x, null_deleter() ); p2->shared_from_this(); // ??
On Sat, 2008-04-26 at 15:45 +0300, Peter Dimov wrote:
Daniel Frey:
I refactored enable_shared_from_this in order to remove the _owner flag.
To test it, you need a small and straight-forward addition to detail::weak_count: It needs .empty(), similar to detail::shared_count.
Are you sure that you need to test for emptiness instead of expiration?
I'm not sure, because I don't know what is expected. I initially used .use_count() != 0, but it triggered a problem in esft_constructor_test.cpp, line 131. I already asked Frank for more information/documentation about what (the new) esft should support, but he said that there is no documentation, instead he pointed me to the regression tests, so I figured I need to make them pass. With .empty(), they do. When looking at the test case that fails with .use_count() != 0, I don't really see why the call to shared_from_this() in line 130 should throw. Shouldn't the call alone be legal? The only illegal thing I could spot in this test case is, that after calling x.shared_from_this() no other shared_ptr takes ownership of x. Regards, Daniel PS: I already committed the fix for the new shared_ptr's ctors. What about adding the corresponding reset()? The refactored esft could use it to simplify one line. And what's your opinion on documenting the new interface? (You've seen the documentation thread?)
Daniel Frey:
When looking at the test case that fails with .use_count() != 0, I don't really see why the call to shared_from_this() in line 130 should throw.
The test looks correct to me. After early_px.reset() and px.reset(), no shared_ptr owns &x. Their initial use_count() is 2, so after the two resets a weak_ptr to &x should expire (it might be a good idea to add it to the test). We'll indeed need weak_count::empty to make this and test5 both pass, though.
On Sat, 2008-04-26 at 18:34 +0300, Peter Dimov wrote:
Daniel Frey:
When looking at the test case that fails with .use_count() != 0, I don't really see why the call to shared_from_this() in line 130 should throw.
The test looks correct to me. After early_px.reset() and px.reset(), no shared_ptr owns &x. Their initial use_count() is 2, so after the two resets a weak_ptr to &x should expire (it might be a good idea to add it to the test). We'll indeed need weak_count::empty to make this and test5 both pass, though.
The current test protects against misuse of shared_from_this, the exception it expects is just a diagnostic for it. My new version even allows one more use case: When the refactored implementation reaches line 130, the weak_count.use_count() == 0 is true, thus init_weak_once() initializes a fresh _shared_count/_weak_count pair and expects someone else to take over ownership later as if the object was just created. Unless I'm missing something... Regards, Daniel
Daniel Frey:
My new version even allows one more use case:
When the refactored implementation reaches line 130, the weak_count.use_count() == 0 is true, thus init_weak_once() initializes a fresh _shared_count/_weak_count pair and expects someone else to take over ownership later as if the object was just created.
Yes. What is the motivation for this use case? Given the sequence: 1. Object is created 2. A shared_ptr takes ownership 3. Owner dies 4. Another shared_ptr takes ownership the original esft allowed shared_from_this calls after 2 and 4, but not after 1 or 3. The new esft allows shared_from_this after 1 as well with the motivation to support calls from within the object constructor. But the constructor only runs once, so this motivation doesn't extend to 3.
On Sat, 2008-04-26 at 19:10 +0300, Peter Dimov wrote:
Given the sequence:
1. Object is created 2. A shared_ptr takes ownership 3. Owner dies 4. Another shared_ptr takes ownership
the original esft allowed shared_from_this calls after 2 and 4, but not after 1 or 3. The new esft allows shared_from_this after 1 as well with the motivation to support calls from within the object constructor. But the constructor only runs once, so this motivation doesn't extend to 3.
OK, understood. The attached version should behave as expected and passes all regression tests. It requires weak_count.empty(). Regards, Daniel
On Saturday 26 April 2008 12:34, Daniel Frey wrote:
OK, understood. The attached version should behave as expected and passes all regression tests. It requires weak_count.empty().
One minor tweak, init_weak_once doesn't seem to need two if statements: void init_weak_once() const { if( _weak_count.empty() ) { detail::shared_count( (void*)0, detail::sp_deleter_wrapper() ).swap( _shared_count ); _weak_count = _shared_count; } } -- Frank
On Sat, 2008-04-26 at 13:20 -0400, Frank Mori Hess wrote:
On Saturday 26 April 2008 12:34, Daniel Frey wrote:
OK, understood. The attached version should behave as expected and passes all regression tests. It requires weak_count.empty().
One minor tweak, init_weak_once doesn't seem to need two if statements:
void init_weak_once() const { if( _weak_count.empty() ) { detail::shared_count( (void*)0, detail::sp_deleter_wrapper() ).swap( _shared_count ); _weak_count = _shared_count; } }
Yepp. I already noticed it as well. :) I think all in all the new version is a good improvement over the former one. It passes the new regression test, it is more efficient and FWIW I find its implementation details much easier to understand than the trunk version. OK to commit? Regards, Daniel
On Saturday 26 April 2008 13:44, Daniel Frey wrote:
I think all in all the new version is a good improvement over the former one. It passes the new regression test, it is more efficient and FWIW I find its implementation details much easier to understand than the trunk version. OK to commit?
It's fine by me. -- Frank
On Sat, 2008-04-26 at 15:22 -0400, Frank Mori Hess wrote:
On Saturday 26 April 2008 13:44, Daniel Frey wrote:
I think all in all the new version is a good improvement over the former one. It passes the new regression test, it is more efficient and FWIW I find its implementation details much easier to understand than the trunk version. OK to commit?
It's fine by me.
Done. I made one more change to the member function sp_accept_owner, hope it's OK... Regards, Daniel
On Saturday 26 April 2008 09:56, Daniel Frey wrote:
On Sat, 2008-04-26 at 15:45 +0300, Peter Dimov wrote:
Are you sure that you need to test for emptiness instead of expiration?
I'm not sure, because I don't know what is expected. I initially used .use_count() != 0, but it triggered a problem in esft_constructor_test.cpp, line 131. I already asked Frank for more information/documentation about what (the new) esft should support, but he said that there is no documentation, instead he pointed me to the regression tests, so I figured I need to make them pass. With .empty(), they do.
When looking at the test case that fails with .use_count() != 0, I don't really see why the call to shared_from_this() in line 130 should throw. Shouldn't the call alone be legal? The only illegal thing I could spot in this test case is, that after calling x.shared_from_this() no other shared_ptr takes ownership of x.
Yes, it seemed safer to make the shared_ptr from shared_from_this() calls share ownership with a well-defined external shared_ptr (that is, the first shared_ptr to take ownership). That seemed to minimize the ambiguity and possibility of dangling shared_ptr. The behavior of the classic esft is different, shared_from_this() returns shared_ptr that share ownership with the last shared_ptr to take ownership. Another possibility, which you seem to be suggesting, would be to always handle the "no owner exists" case by having shared_from_this() return shared_ptr that will share ownership with the next external shared_ptr to take ownership. In any case, the exact behavior of shared_from_this wrt multiple owners has never been specified as far as I know. -- Frank
Frank Mori Hess:
In any case, the exact behavior of shared_from_this wrt multiple owners has never been specified as far as I know.
It hasn't, but the case being discussed doesn't involve multiple (simultaneous) owners. The behavior of the original enable_shared_from_this is well defined when there is a single owner; the behavior of the Boost implementation was also predictable when there was no owner. (It's undefined in TR1, though.)
Daniel Frey:
I refactored enable_shared_from_this in order to remove the _owner flag.
There's another problem with your new implementation, one that wasn't caught by esft_regtest (I updated it to catch it): void test4() { boost::shared_ptr<V> pv( new V ); boost::shared_ptr<V> pv2( pv.get(), null_deleter() ); } Your implementation modifies pv2. This ease with which one might inject subtle failures in remote code (such as Boost.Python, incidentally) makes me think that sp_accept_owner is a bit too dangerous. Maybe its shared_ptr argument needs to be made const. This would break the new constructorful esft though.
On Sat, 2008-04-26 at 16:48 +0300, Peter Dimov wrote:
Daniel Frey:
I refactored enable_shared_from_this in order to remove the _owner flag.
There's another problem with your new implementation, one that wasn't caught by esft_regtest (I updated it to catch it):
void test4() { boost::shared_ptr<V> pv( new V ); boost::shared_ptr<V> pv2( pv.get(), null_deleter() ); }
Your implementation modifies pv2.
The attached version should fix that. It also passes test5() - unlike the current trunk version which uses _owner. FWIW, it still fails to pass the esft_constructor_test.cpp, line 131. Regards, Daniel
On Saturday 26 April 2008 09:48, Peter Dimov wrote:
This ease with which one might inject subtle failures in remote code (such as Boost.Python, incidentally) makes me think that sp_accept_owner is a bit too dangerous. Maybe its shared_ptr argument needs to be made const. This would break the new constructorful esft though.
Users can already define their sp_accept_owner to take a const argument, and could be encouraged to do so by the documentation. And if we did a const_cast of "this" to make it const before passing it to sp_accept_owner, a determined user could just as easily reverse it with another const_cast. -- Frank
participants (3)
-
Daniel Frey -
Frank Mori Hess -
Peter Dimov