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