[boost] Re: Boostifying Loki::SmartPtr

From: Andrei Alexandrescu (andrewalex_at_[hidden])
Date: 2002-04-19 20:18:59

"Gennadiy Rozental" <rogeeff_at_[hidden]> wrote in message
news:a9obtj$iev$1_at_main.gmane.org...

I'm about to get really busy starting... 3, 2, 1,... now, so I'll have to
reply much more sparsely for a while. I do want to take time and comment on
Gennadiy's excellent analysis. Unlike my habit, I will copy all of his
points and I'll add an "Agreed" to reinforce that Gennadiy and I have the
same view.

> 1. Start with framework and then put specific policies. I think it clarify
> code reading.

Agreed.

> 2. Template template parameters:
> template <class> class OwnershipPolicy,
> It  does not bring to much value but immidiately prevent compilation on
> several compilers.
> Accordingly stop using this wierd types: SP, OP, KP

I understand that this is a portability issue. Grudgingly agreed. Hopefully
we can keep things easy for the user of smart_ptr.

> 3. Defined types:
> typedef typename SP::reference_type reference_type;
> Should also define const_reference_type. See below.

Agreed. The whole part of pointers to const hasn't been thoroughly
implemented.

> 4. destructive_copy
> typedef typename Select<OP::destructive_copy, smart_ptr, const
> smart_ptr>::Result
> copy_arg;
> Does not seems reasonable. What will I do with auto_ptr that expect
> non-const argument for assingment and copy construction? That mean it will
> be mutable in any class that contain it. I better do once const_cast. So
> hole logic about destructive_copy should be removed. So no need t oworry
> about Select facility.

This constructor was carefully designed (read: hacked for many nights) to
fully emulate the std::auto_ptr design (read: amazingly intelligent hack). I
suggest we keep it so that smart_ptr remains a borg that swallows all other
pointers, auto_ptr included :o).

> 5. smart_ptr(const stored_type& p) constructor
> Significant framework flaw: does not support intrusive reference counting
> smart_ptr(const stored_type& p) : SP(p)
> { KP::on_init(get_impl(*this)); }
> OwnershipPolicy is not able to update counter here. Here my version:
> smart_ptr( stored_type const& p ): StoragePolicy( p )
> {
>     CheckingPolicy::on_init( StoragePolicy::storage() );
>
>     OwnershipPolicy::init_reference( StoragePolicy::storage() ); // for
> storage() see below
> }
> So Ownership policy should support another interface : init_reference

Well that's a tough call. My initial design was assuming that in the case of
intrusive reference counting, the class initializes its own reference
counter. I guess there are pros and cons either way, so it's not really a
flaw. If we are to pass the buck to the OwnershipPolicy, I suggest we do
that in the constructor.

> 6.  I disagly completely with Copy Contructor logic
> For for reasoning and explanations see reference before in this thread.
Here
> my version:
> smart_ptr( smart_ptr const& ptr ) // always const
> : StoragePolicy( ptr ),  // here the real copying is performed
>   OwnershipPolicy( ptr ),
>   ConversionPolicy( ptr ),
>   CheckingPolicy( ptr )
> {
>     if( OwnershipPolicy::add_reference( StoragePolicy::storage() ) ) //
only
> add reference
>         const_cast<smart_ptr&>(ptr).release();                    //
> const_cast to be able to call release()
> }
>
> So instead of Clone method OwnershipPolicy::should support add_reference
> method
> The same for all copy contructors.

Hmmm... there are several interesting points here.

One is that disagreeing with the parameter type of the copy cosntructor
basically boils down to disagreeing with auto_ptr. I guess that's a judgment
call (a very bright and marcant member of the C++ community whom I know
really hates auto_ptr and thinks it's a catastrophe).

But I do think, again, that auto_ptr compatibility is an important goal.

The other point is that "clone" is a misnomer and it better should be
"add_reference". However, for an eager-copying smart pointer, and for pretty
much anything but refcounting, add_reference doesn't make sense - it
actually clones the object. What would be a better name than both?

> 7. Could you explain value of by_ref and everything related to it?

One word: auto_ptr. In particular, by_ref is a nice concept that I also use
in ScopeGuard and somewhere else (I forgot where). Ah, I know: In Functor
when I want to bind a reference.

> 8. Assingment operator should use reset intead of logic duplicating. It
> should ot be using template template parameters.

Could you detail here a little?

> 9. if BOOST_NO_MEMBER_TEMPLATES is defines there is no
>     smart_ptr&      operator=( smart_ptr const& ptr )

There should be, is that what you mean?

> 10. Destructor. should follow the constructor. I would prefer name
> release_reference intead of relase in OwnershipPolicy. Will sould close to
> other methods.

Ok.

> 11. I did not buy arguments abour get and release, so I would prefer them
to
> be smart_ptr methods.

Disagree, but I'm out of new arguments.

> 12. I could be missing something but I do not see how release(...)
releasing
> reference. Looks like significant framework flaw. Also I would like
> CheckingPolicy to check stored_value before releasing. So here my version:
>
> stored_type     release()
> {
>     CheckingPolicy::on_release( StoragePolicy::storage() );
>     OwnershipPolicy::release_reference( StoragePolicy::storage() );
>     stored_type res = StoragePolicy::storage();
>
>     StoragePolicy::storage() = StoragePolicy::default_value();
>     return res;
> }

I don't think your version is correct. smart_ptr::release's semantics is, it
simply relinquishes ownership of the resource without touching anything
else. That's why I don't think release() should call release_reference();
that is user's responsibility. Think of this for example: I have an COM
intrusive refcounted pointer with a refcount of one. I want to get the
internal pointer and get rid of the smart pointer. I call release(), but
voila! The release_reference() call calls COM's Release() method on the
pointee, which deletes the object! You get the shell of a dead object.

So I support the original implementation (with the checking that you added):
release() just gives the pointee away and initializes the smart pointer with
the default value. A bug in my implementation is that I didn't reinitialize
the Ownership policy.

By the way, I saw a nice name here for release(): leak().

> 13. Also I would like CheckingPolicy to check stored_value before giving
> direct access.So here my version of get():
>
> pointer         get() const
> {
>     CheckingPolicy::on_direct_access( StoragePolicy::storage() );
>     return StoragePolicy::get_pointer();
> }

Cool.

> 14. operator->() and operator*()
> They both implemented through appropriate function of StoragePoilicy. But
> these methods of StoragePolicy never used as an operators. So why don't
give
> them readable names. I choosed get_pointer() and get_reference();

Cool.

> 15. const operator->() and operator*()
> Why don't they return const_pointer and const_reference appropriately? int
> const& would not allow you to change the referenced object.

Cool.

> 16. operator!()
> Significant framework flaw:
> bool operator!() const
> { return get_impl(*this) == 0; } //Why do you compare to 0? It could be
not
> a pointer at all!
> Here is my version:
> bool            operator!() const
> {
>    return !StoragePolicy::is_initialized();
> }
> So StoragePolicy need new interface is_initialized();

Cool. I think we could compare against SP::default_value().

> 17. Automated conversion.
> I prefer more flexible solution when ConversionPolicy will define
> automatic_conversion_result_type.
> To disallow conversion I am using following ConvertionPolicy:
> template<class Pointer>
> class disallow_conversion{
> protected:
>     struct disallow_conversion_result {
>         disallow_conversion_result( Pointer& ) {}
>     };
>
>     typedef disallow_conversion_result automatic_conversion_result_type;
>     static void     swap( disallow_conversion& ) {}
> };
> It will require Pointer type as a template for convertion policy.

Sounds great.

> 18. StoragePolicy contract.
> I do not like get_impl, get_impl_ref methods names. I prefer get_pointer,
> get_reference. Though it metter of taste. Also they should be regular
> methods (not free friend functions)

I used the same names in the beta, but then I realized SmartPtr can actually
be used as a smart resource, so I tried to be more fair with these names.

> 19  default_storage_policy
> All methods should be protected.

Igen.

> 20 allow_convertion.
> I am using following class instead:
> template <class ResultType>
> class allow_conversion_to{
> protected:
>     typedef ResultType automatic_conversion_result_type;
>
>     static void     swap( allow_conversion_to& )            {}
> }

Good.

> 21. CheckingPolicy.
> I prefer idfferent implemetation for all CheckingPolicies. I think it more
> convinient and powerful. For more detail see complete implementation.

Ok.

> 22. OwnershipPolicies
> a. ref_counted - I prefer nonintrusive_ref_counted
> b. deep_copy - As I explained in the message referenced before in this
> thread deep_copy is part od StoragePolicy functionality. Approprite
> OwnerShip policy I named confident_owner. For details see complete
> implementation.
> c. destructive_copy.
> OwnershipPolicy should not know how to perform a copy. it part of
> StoragePolicy functionality. To define auto_ptr like smart_pointer a am
> using transferrable OwnershipPolicy. See complete implementaion for
details.
> d. no_copy - I prefer name non_sharable.

I recall I did find that unnatural as well, and I tried to do it the way you
say. I then realized that the ownership sometimes might dictate whether a
copy or a simple sharing is made, so I passed the whole copying
responsibility to it. It seems like you got that better, I'll have to check
the source code.

Hey, seems like things are going well! Thanks Gennadiy.

Andrei