Re: [boost] [review] Formal Fast-Track review of the Boost.Functional/Factory library - extended until sunday

From: Stjepan Rajko (stipe_at_[hidden])
Date: 2007-12-21 14:31:22

On Dec 21, 2007 5:33 AM, John Torjo <john.groups_at_[hidden]> wrote:
>
> What to include in Review Comments
> ==================================
>
> * What is your evaluation of the design?

Nice and clean.  One complaint: Allocator should be inherited :-)  OK,
in general, here is a case for supporting inheritance in all of these
adapter classes.  As an exercise, I wanted to make a factory that will
do two things - one, provide perfect forwarding; and two - count the
number of objects that have been created by the factory.

This is what I came up with:

template<typename T, typename Allocator=std::allocator<void> >
struct counting_factory : private boost::factory<T, Allocator>
{
    counting_factory() : count() {}

    using counting_factory::result_type;

    template<typename Sequence>
    typename counting_factory::result_type operator()(const Sequence &s)
    {
        count++;
        return boost::fusion::fused<boost::factory<T, Allocator> >()(s);
    }

    int count;
};

template<typename T, typename Allocator=std::allocator<void> >
struct perfect_factory
    : public boost::forward_adapter<
        boost::fusion::unfused_lvalue_args<
            counting_factory<T, Allocator>
        >
    >
{};

There are two problems.  One - I can't dig out the count, because it's
buried in private membership with no access functions to let me get to
it.  Two - the size of the resulting perfect_factory is 8 (GCC
4.0.1/darwin), and as far as I can tell the only thing in this entire
chain of inheritance that needs storage space is `int count`.

Maybe there is a simple way to do what I set out to do without running
into the above problems, in which case I'd like to know about it.
Otherwise, I would jump with joy if all of these adapters used
*protected* inheritance so I could also dig up parts of an inherited
class's interface (say, counting_factory above could have a public
method get_count, which I could expose as public in the
perfect_factory).

> * What is your evaluation of the implementation?

Didn't see any problems with this one. But, in testing it I think I
found that forward_adapter.hpp needs a #undef BOOST_PP_INDIRECT_SELF

> * What is your evaluation of the documentation?

When I first gave this one a try, I skimmed through the docs and
didn't realize that

* factory wants a pointer type as the template paramater
* the pointer can be a smart pointer (this is really cool)

Of course, it's all there, but to help people like me that like to
skim maybe separate the the two templates (factory and value_factory)
a little bit more in the introduction so that it reinforces that they
are different in usage.

Also, the usefulness of the library is described in "These templates
make other facilities that operate on function objects (such as
standard algorithms, Boost.Bind, Boost.Function, or Fusion functional
adapters) applicable to constructors".  There is a small example below
showing how it can be used with Boost.Function - it would be great to
give some more examples involving bind or fusion adapters, as it would
really help users see what all can be done using this.

The synopsis for factory doesn't mention the Allocator template
parameter... in fact I'm not sure whether this is discussed anywhere.

> * What is your evaluation of the potential usefulness of the library?

I can see it being very useful.

> * Did you try to use the library? With what compiler?

Yes, GCC 4.0.1/darwin.

>    Did you have any problems?

Nope

> * How much effort did you put into your evaluation?
>    A glance? A quick reading? In-depth study?

I skimmed the docs, looked at the code, tried using it a little,
re-read the docs, then played with the example I mentioned above.

> * Are you knowledgeable about the problem domain?
>

I recently found a need for something like this, but otherwise no.

> And finally, every review should answer this question:
>
> * Do you think the library should be accepted as a Boost library?
>    Be sure to say this explicitly so that your other comments
>    don't obscure your overall opinion.
>

Yes, assuming this isn't covered in boost already (Shunsuke Sogame
raised an interesting question in regards with
lambda::constructor/new_ptr.  AFAICT, it does appear similar to
factory, but I'm not sure that new_ptr supports custom Allocators or
smart pointers (I only gave it a brief look).  What about
value_factory<T> and constructor<T>, though?)

Stjepan