Re: [boost] optional and typed_in_place_factory

From: Fernando Cacciola (fernando.cacciola_at_[hidden])
Date: 2008-02-19 16:11:02

Hi James,

Sorry for the late response...

> I've run into problems using the typed_in_place_factory and optional
> objects.
>
> First, I ran into a problem where I initialized a typed_in_place_factory
> with a temporary and then used the factory after the temporary was
> destroyed. After a few hours of hair pulling, I figured out that the
> factory currently only stores references. This was a surprise because I
> have been using boost::bind for quite a while and it holds copies of the
> arguments. I propose that the typed_in_place_factory hold copies of the
> data as well. Users can pass boost::ref objects for types that aren't
> copyable with the added benefit that they explicitly know they need to
> keep the referenced object alive while the factory exists. Consistency
> between the behavior of boost::bind and the typed_in_place_factory is
> desirable because of their similar nature.
>
I need to see the history of bind to learn why it holds a copy of arguments, 
which off the top of my head seems second best.

Anyway...

After 1.35 is out, and when I have enough time, I will refactor the design 
of this in place factories based on suggestions made by Corrado Zoccolo.
Basically, the application of the factory will be separated from the list of 
arguments.
That will allow the list of arguments to be a concept with varying 
representations, including versions that hold copies of arguments instead of 
references, for instance.

The new design will have to be backward-compatible with the current 
interface and behaviour, of course, so at most a deep version of the 
argument list will be an alternative, but not the default.

> Second, I've noticed that operator= for the optional takes a factory
> object by value. It seems like taking the argument by const reference
> would be desirable to prevent copying of the factory (which could be
> expensive if the changes to the typed_in_place_factory mentioned
> previously are accepted).
>
> Also, the conversion constructor takes the
> factory by const reference, so this may have just been a simple oversight.

Oh.. yes, that's an oversight.

>
> Below is a patch based on HEAD as of 2007-01-29.
>
> --------------------------------------------------------------
>
> Index: boost/optional/optional.hpp
> ===================================================================
> RCS file: /cvsroot/boost/boost/boost/optional/optional.hpp,v
> retrieving revision 1.12
> diff -p -r1.12 optional.hpp
> *** boost/optional/optional.hpp 26 Jun 2006 18:01:36 -0000      1.12
> --- boost/optional/optional.hpp 29 Jan 2008 18:52:11 -0000
> *************** class optional : public optional_detail:
> *** 510,516 ****
>       // Assigns from an expression. See corresponding constructor.
>       // Basic Guarantee: If the resolved T ctor throws, this is left
> UNINITIALIZED
>       template<class Expr>
> !     optional& operator= ( Expr expr )
>         {
>           this->assign_expr(expr,&expr);
>           return *this ;
> --- 510,516 ----
>       // Assigns from an expression. See corresponding constructor.
>       // Basic Guarantee: If the resolved T ctor throws, this is left
> UNINITIALIZED
>       template<class Expr>
> !     optional& operator= ( Expr const& expr )
>         {
>           this->assign_expr(expr,&expr);
>           return *this ;

I'll apply this fix after 1.35 is out, just make sure (though it should 
matter if I fix the trunk only)

> Index: boost/utility/detail/in_place_factory_prefix.hpp
> ===================================================================
> RCS file:
> /cvsroot/boost/boost/boost/utility/detail/in_place_factory_prefix.hpp,v
> retrieving revision 1.2
> diff -p -r1.2 in_place_factory_prefix.hpp
> *** boost/utility/detail/in_place_factory_prefix.hpp    26 Jun 2007
> 23:07:25 -0000      1.2
> --- boost/utility/detail/in_place_factory_prefix.hpp    29 Jan 2008
> 18:52:13 -0000
> ***************
> *** 26,32 ****
>   #include <boost/preprocessor/repetition/enum_trailing_params.hpp>
>
>   #define BOOST_DEFINE_INPLACE_FACTORY_CLASS_MEMBER_INIT(z,n,_)
> BOOST_PP_CAT(m_a,n) BOOST_PP_LPAREN() BOOST_PP_CAT(a,n) BOOST_PP_RPAREN()
> ! #define BOOST_DEFINE_INPLACE_FACTORY_CLASS_MEMBER_DECL(z,n,_)
> BOOST_PP_CAT(A,n) const& BOOST_PP_CAT(m_a,n);
>
>   #define BOOST_MAX_INPLACE_FACTORY_ARITY 10
> --- 26,32 ----
>   #include <boost/preprocessor/repetition/enum_trailing_params.hpp>
>
>   #define BOOST_DEFINE_INPLACE_FACTORY_CLASS_MEMBER_INIT(z,n,_)
> BOOST_PP_CAT(m_a,n) BOOST_PP_LPAREN() BOOST_PP_CAT(a,n) BOOST_PP_RPAREN()
> ! #define BOOST_DEFINE_INPLACE_FACTORY_CLASS_MEMBER_DECL(z,n,_)
> BOOST_PP_CAT(A,n) BOOST_PP_CAT(m_a,n);
>
>   #define BOOST_MAX_INPLACE_FACTORY_ARITY 10
>

And this fix will serve as the basis for the future deep version of the 
argument list.

Thank you for the report

-- 
Fernando Cacciola
SciSoft
http://fcacciola.50webs.com
http://groups.google.com/group/cppba