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

From: Tobias Schwinger (tschwinger_at_[hidden])
Date: 2007-12-21 19:19:50

Hi Stjepan,

thanks a lot for your both detailed and positive review!

Stjepan Rajko wrote:
> 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 :-)  

Yes, that's a very good point (much more than with Forward, IMO).

However, I intentionally left support for custom Allocators undocumented
to see whether someone would miss it. It doesn't really feel right to me
and I'm tempted to throw it out again:

If we use a custom Allocator we'll also need a custom Smart Pointer that
uses this Allocator. None of our Boost Smart Pointers support Allocators
and the Factory should actually initialize a stateful Allocator within a
Smart Pointer that does.

Overloading 'new' as a static member function seems the better way to go
and one can write a template that does it inheriting an arbitrary class.

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

Thanks for spotting this one!

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

OK.

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

Yes, I figured this would be helpful answering Nat Goodspeed's review.

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

See above.

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

See answer to Shunsuke's post.

Regards,
Tobias