[boost] Re: Boostifying Loki::SmartPtr

From: Gennadiy Rozental (rogeeff_at_[hidden])
Date: 2002-04-20 01:30:45

"Andrei Alexandrescu" <andrewalex_at_[hidden]> wrote in message
news:a9qfpa$6ld$1_at_main.gmane.org...
> "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.
>
> > 2. Template template parameters:
> > template <class> class OwnershipPolicy,
> > It  does not bring to much value but immediately prevent compilation on
> > several compilers.
> > Accordingly stop using this weird 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.

As an alternative we can consider hiding using template template parameters
under ifdef BOOST_NO_TEMPLATE_TEMPLATE ( is there one?)

> > 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 assignment 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 to worry
> > 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).

I do not have an access to the standard, but all implementation I have
access to and draft standard I found online use const auto_ptr<T>& as a copy
constructor argument.

> > 6.  I disagree completely with Copy Constructor logic
> > 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 constructors.
>
> Hmmm... there are several interesting points here.
>
> One is that disagreeing with the parameter type of the copy constructor
> 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?

No it never perform real copying. That is my major point! Look here for
details:
http://groups.yahoo.com/group/boost/message/25141
BTW: How did you envision to support deep copy for arrays?

>
> > 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.

Could you provide an examples. Is it required to be part of smart_ptr
interface?

> > 8. Assignment operator should use reset instead of logic duplicating. It
> > should not be using template template parameters.
>
> Could you detail here a little?

Here you version:
        smart_ptr& operator=(copy_arg& rhs)
        {
            smart_ptr(rhs).swap(*this);
            return *this;
        }

I propose:

        smart_ptr& operator=(copy_arg& rhs)
        {
            reset( rhs );
            return *this;
        }

> > 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?

In version presented by Dave there is not.

> > 11. I did not buy arguments about get and release, so I would prefer
them
> to
> > be smart_ptr methods.
>
> Disagree, but I'm out of new arguments.

My reasons:
1. As you mention in the book: "Overloaded function can be just as confusing
as member function". I.e. let say you have smart_ptr that holds another
smart_ptr. then you could easily mistype
release( *sp ) and release( sp ), they both seems reasonable.
2. As you emphasized twice we want to support auto_ptr interface: it has
get() and release()
3. boost::shared_ptr provide get()
4. A lot of people are used to this syntax

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

That's funny. today I spent a lot of time tracking bug caused by misuse of
method release().
Yes I get a good point. I do behave incorrectly. But originally I was trying
to solve an following interface problem: given let say non intrusive
reference counting after release() how will I decrement counter. Or more
generally what OwnershipPolicy should do during release. I assume we will
need new interface to OwnershipPolicy: leak_reference(). Then the method
will look like this:

stored_type     leak()
{
     CheckingPolicy::on_leak( StoragePolicy::storage() );
     OwnershipPolicy::leak_reference( StoragePolicy::storage() );

     stored_type res = StoragePolicy::storage();
     StoragePolicy::storage() = StoragePolicy::default_value();
     return res;
}

I like name leak but ... auto_ptr ... compatibility ... heh.

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

I prefer is_initialized. You can't be sure that == default_value is correct.

> > 18. StoragePolicy contract.
> > I do not like get_impl, get_impl_ref methods names. I prefer
get_pointer,
> > get_reference. Though it matter 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.

But you still have pointer_type, reference_type. We just should agree that
we using this terms in generic sense. I.e. pointer_type is type returned by
operator->() reference_type - by operator*() and so on.

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

I could present version with my comments and put it in vault area.

Gennadiy.

P.S. I forgot one item. I propose to separate smart_ptr.hpp onto 3 headers
to minimize dependencies:
1. smart_ptr.hpp
2. smart_ptr_compare.hpp - contain everything related to comparisons. in
most cases we do not need it. And it also bring 2 stl headers.
3. smart_ptr_checkers.hpp - will include all custom checkers and appropriate
stl/system headers.