[boost] Re: Boostifying Loki::SmartPtr

From: Gennadiy Rozental (rogeeff_at_[hidden])
Date: 2002-04-19 01:02:57

Hi,

Here itemized list of my complains:

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

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

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

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.

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

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.

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

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

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

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

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

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;
}

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();
}

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();

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.

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();

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.

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)

19  default_storage_policy
All methods should be protected.

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& )            {}
}

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

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.

Implementation mention in the body of the message located here:
http://groups.yahoo.com/group/boost/files/PolicyBased%20smart_ptr/smart_ptr.
h

As I sad before, be aware that code was intended to work for old Sun C++
4.2, MSVC 6 and gcc 2.91 ( it does ). That explain some weirdness you will
see.

Regards,
Gennadiy.