From: Tobias Schwinger (tschwinger_at_[hidden])
Date: 2007-12-21 19:19:50
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.
> 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.
>> * 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk