Boost logo

Boost :

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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk