[review] Formal Fast-Track review of the Boost.Functional/Factory library - extended until sunday
Hi all, I've extended the review until Sunday (inclusive). I'm hoping for more reviews. So far, we've had only two. If you are interested in the library (you should be :)), please take some time to review it. This is the info you need to do a review on: *Description* Factories are callback mechanisms for constructors, so we provide two function objects, boost::value_factory and boost::factory, that encasulate object construction through direct application of the constructor or operator new, respectively. 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. *Author* Tobias Schwinger *Download* Get it from here: http://www.boost-consulting.com/vault/index.php?action=downloadfile&filename=factory.zip&directory=X-Files& Read documentation online here: http://torjo.com/tobias/ What to include in Review Comments ================================== Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? 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. Best, John Torjo - Review Manager - -- http://John.Torjo.com -- C++ expert ... call me only if you want things done right
On Dec 21, 2007 5:33 AM, John Torjo <john.groups@torjo.com> 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
On Dec 21, 2007 12:31 PM, Stjepan Rajko <stipe@asu.edu> wrote:
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
Hmm... I'm not sure that I know exactly what I'm talking about when insisting the inheritance should be *protected*... maybe just ignore it ... serves me right for bolding something like that :-) But this did make me think about whether using compressed_pair is the right thing to do, because maybe there are cases where things would be useful as inherited even when the base class is not empty. Stjepan
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@torjo.com> 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
Tobias Schwinger wrote:
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.
Doesn't shared_ptr support a custom deleter? A custom deleter is just an allocator by another name/interface IMO... John.
John Maddock wrote:
Tobias Schwinger wrote:
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.
Doesn't shared_ptr support a custom deleter? A custom deleter is just an allocator by another name/interface IMO...
Thanks! Well, this feature of 'shared_ptr' sets the situation into a different light (is it new?)... I'm considering a (partial) specialization for the Allocator template parameter, assuming a second constructor parameter for Smart Pointers in case 'Allocator' is not void (figuring 'std::allocator' is still an Allocator and should be treated as such). Regards, Tobias
Tobias Schwinger wrote:
John Maddock wrote:
Tobias Schwinger wrote:
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.
Doesn't shared_ptr support a custom deleter? A custom deleter is just an allocator by another name/interface IMO...
Thanks!
Well, this feature of 'shared_ptr' sets the situation into a different light (is it new?)...
I don't think so, it's certainly extremely useful, as amounst other things supplying a custom deleter doesn't change the pointer type :-)
I'm considering a (partial) specialization for the Allocator template parameter, assuming a second constructor parameter for Smart Pointers in case 'Allocator' is not void (figuring 'std::allocator' is still an Allocator and should be treated as such).
I think that's correct: some std::allocators may do more than just call ::new, I don't know if it still does, but STLport's used to cache memory for example. John.
participants (2)
-
John Maddock -
John Torjo -
Stjepan Rajko -
Tobias Schwinger