Re: [boost] [type_erasure] Review started (July 18-27, 2012)

Subject: Re: [boost] [type_erasure] Review started (July 18-27, 2012)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2012-08-03 14:39:54

AMDG

On 08/02/2012 12:29 PM, Larry Evans wrote:
> On 07/18/12 00:13, Lorenzo Caminiti wrote:
>> Hello all,
>>
>> *** The review of Steven Watanabe's proposed Boost.TypeErasure library
>> begins on July 18, 2012 and ends on July 27, 2012. ***
> [snip]
> Should type_erasure be accepted into Boost?
> 
>   Answer:
>     No, not yet.
>   Why(in order of importance):
> 
>     1) any<C,T>::any(U&, static_binding<Map>&) bug
> 
>        The bug reported here:
>          http://article.gmane.org/gmane.comp.lib.boost.devel/233101
>        should be resolved.
> 
>        The bug is just too dangerous for the resolution to be delayed.
> 

I added a static assert.

> <snip>
> 
> My review:
> 
> 1. What is your evaluation of the design?
> 
>    Good, except for:
> 
>      [need variadic static_binding ctor]:
> 
>        The any CTOR:
> 
>          template<class U, class Map>
>          any(U & arg, const static_binding< Map > & binding);
> 
>        documented here:
> 
> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/doc/html/boost/type_erasure/any_Concept__T___id2445315.html#id2445396-bb
> 
>        allows only a single argument to the constructor to the
>        underlying type.  I'd suggest adding another CTOR with
>        interface:
> 
>          template<class Map, class... U>
>          explicit any(const static_binding< Map > & binding, U &&... arg);
> 
>        which is similar to:
> 
>          explicit any(const binding< Concept > & binding, U &&... arg);
> 
>        Then any constructor of the underlying could be used.
> 

I think something like this is reasonable.
I have to consider whether this is best
approach, though.  I don't consider this
high priority, however, since the copy
constructor should work most of the time.
The STL got by just fine for years without
emplace.

>      [too many any ctors]:
> 
>        According to `grep -e '   any('`, there are more than 45 any
>        constructors.  That many makes it hard for the novice to know
>        which one to use.  The  Design notes:
> 

The solution is simple.  Don't use the source.
Use the documentation page for any, which only
shows 9 distinct constructors.

> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/doc/html/boost_typeerasure/design.html#boost_typeerasure.design.constructors
> 
>        are some help, but those notes also mention ambiguities and
>        user rules, which sound like indications of usability problems
>        to me.
> 
>        Why wouldn't the following ctors be sufficent to do all the
>        current any constructors do (except for maybe requiring more
>        typing and maybe more constructor calls)?
> 
>            template
>            < class... U
>            >
>          any
>            ( static_binding<Map>const&
>            , U&&...
>            );//kind=binding constructor
> 
>            template
>            < class... U
>            >
>          any
>            ( binding<Concept>const&
>            , U&&...
>            );//kind=dispatching constructor
> 
>            template
>            < class Concept2
>            , class Tag2
>            >
>          any
>            ( any<Concept2,Tag2>U&&
>            , static_binding<Map>const&
>            );//kind=converting constructor
> 
>            template
>            < class Concept2
>            , class Tag2
>            >
>          any
>            ( any<Concept2,Tag2>U&& u
>            , binding<Concept2>const&b=binding_of(u)
>            );//kind=converting constructor
> 
>        Providing just these 4 constructors would improve the usabilty.

Why?

>        Of course, I'm probably missing something, and if so,
>        explaining why the above would not work in the Design notes
>        would be helpful.
> 

I think it should be obvious what's missing:

a) any(const any&);
   The copy constructor is not a template.
b) template<class U> any(const U&);
   This is the most commonly used constructor
   besides the copy constructor.  It would
   be very inconvenient if it didn't exist.
c) any();
   The default constructor creates an empty any.
d) A default argument can't refer to a prior
   argument.  Your fourth constructor has to
   be two separate constructors.

> 2. What is your evaluation of the implementation?
> 
>    [in-source comments]:
>      Very few in-source comments, as for example, the in the code:
> 
> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/boost/type_erasure/detail/normalize.hpp
> 
>      there are no in-source comments. This lack of in-source comments
>      made it very hard to understand the implementation. There is also
>      no tests in the directory:
> 
> 
> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/test/
> 
>      which contain any string with 'normalize' in it; hence, I assume
>      there are no unit tests for any class or function with normalize
>      as part of its name.  Such unit tests, hopefully, would have
>      provided more information of the purpose of the code in
>      */detail/normalize.hpp; hence, I'd recommend providing such tests
>      in addition to more in-source comments.
> 

The tests only cover the public interface.
I'm not going to add specific tests of
the implementation details.

> 
> 3. What is your evaluation of the documentation?
> 
>    Mostly good, but some documentation needs improvement.
>    For example:
> 
>    [vague:rebinding_any.html]:
> 
>      This doc:
> 
> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/doc/html/boost/type_erasure/rebind_any.html
> 
>      Claims rebind_any<Any,T>::type:
> 
>        returns any type corresponding to a placeholder.
> 
>      However, it doesn't say what the Any in rebind_any<Any,T> may be,
>      and if one assumes Any is a type_erasure::any<C,S> for some
>      concept, C, and placeholder, S, then that description makes no
>      sense because C does not contain any information about which
>      actual type is matched to which placeholder.

Huh?  As the documentation you quoted
says, rebind_any returns a specialization
of /any/.  rebind_any<any<C, S>, T>::type
is any<C, T>.

> <snip>
> 
>      from Steven, in the reply:
> 
>        http://article.gmane.org/gmane.comp.lib.boost.devel/233067
> 
>      did I finally conclude:
> 
>        The placeholders in concepts do *not* stand for the bound types
>        but instead for the any's containing those bound types.
> 

That depends on the context.
boost::type_erasure::call "unwraps"
these any's.

>      Which is similar to the conclusion which I posted here:
> 
>        http://article.gmane.org/gmane.comp.lib.boost.devel/233078
> 
>      Since the 233078 post got no replies, I assume that conclusion is
>      right.  If so, then that should be made clear in the
>      documentation (including tutorials).  In addition, the following
>      example code (and especially the comments), should reduce future
>      confusion on this topic:
> 
> /* {***any_ctor_kinds.cpp*** */
> <snip>
> 

I find this code almost completely unreadable.

>      If that conclusion is wrong, then the docs need to be modified
>      (I've no idea how) to clearly define "matching" as used in the
>      any.html#id2445089-bb reference.
> 
>    [incomplete:tuple.html]:
> 
>      The doc:
> 
> 
> http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/doc/html/boost/type_erasure/tuple.html
> 
>      makes no mention of how the binding produced by this call are
>      generated.
> 

Already fixed.

In Christ,
Steven Watanabe