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

Hi Stjepan, thanks a lot for your both detailed and positive review! Stjepan Rajko wrote:

On Dec 21, 2007 5:33 AM, John Torjo 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