Re: [boost] Phoenix review

Subject: Re: [boost] Phoenix review
From: Joel de Guzman (joel_at_[hidden])
Date: 2008-09-29 23:45:32

Peter Dimov wrote:
> I apologize in advance that I could not dedicate the appropriate amount 
> of time for my evaluation of the library.
> 
> * Design
> 
> - The placeholders probably belong in namespace phoenix::placeholders 
> instead of arg_names, for consistency with std::placeholders.

Ok.

> - It is very odd to include phoenix_bind.hpp and not have the 
> placeholders available. For that I need phoenix_core.hpp.

Hmm.. I missed that. Ok.

> - The decision to capture the left side of @= and = by value is 
> debatable. It is indeed not clear which one is 'better', but we do have 
> Lambda in Boost, and to deviate from it one needs a stronger supporting 
> argument than that.

Consistency is my prime argument for always capturing by value.
You have a good point that cout (and arrays) are being captured
by reference and thus violates this consistency. But IMO, it's
not the same. There can be exceptions to the rule and that
exception only extends so far as the type of an object,
not wholesale. And, more importantly, the exceptions are
applied consistently (i.e cout is always by-ref regardless where
it is placed in an expression).

> - In general, the subtle differences between Bind, Lambda and Phoenix 
> need an explanation, even if its only purpose is to clearly show that 
> the deviation is deliberate and not an accident.

Ok.

> - bind<R> is not supported.

Right. Noted.

> - Smart pointers do not seem to be supported well in conjunction with 
> bind, even though they seem to work in other contexts.

I just realized that in the review. ->* supports smart pointers.
There's no reason why bind should not. I consider this a bug.

> - What is the equivalent of boost::unlambda in Phoenix? 

None yet.

> I think I agree 
> with Giovanni Piero Deretta that lambda[] is an intuitive way to put a 
> 'wall' between the inside lambda expression and the outside world. It 
> doesn't seem to; the following fails:
> 
>    px::bind( px::lambda[ arg1 + arg2 ], 0, 1 )();

Yes, that's taken into high priority consideration.

> - It is not clear what introspection facilities are provided. Users 
> sometimes want to derive a signature from a bind expression, or to 
> compute the minimum arity of a lambda expression. They ought to be able 
> to implement these queries themselves via a documented introspection 
> interface that will not break when V3 arrives.

Nice point!

> * Implementation
> 
> - Phoenix objects do not take rvalues. This is a very common stumbling 
> block for people wanting to experiment with lambda libraries.

Yes.

> - boost::is_placeholder needs to be specialized for the placeholder 
> types. It might be interesting to see Phoenix respecting is_placeholder 
> in the other direction, too, if possible.

Ok.

> - The test coverage seems relatively sparse. I know very well how 
> painful it is to write tests (for arities from 0 to 9 inclusive), but 
> Phoenix 2.0 needs and deserves more of them. Take this, for example:
> 
>    test::x x_;
>    bind(&test::x::m, x_)() = 123;
>    bind(&test::x::m, arg1)(x_) = 123;
>    BOOST_TEST(x_.m == 123);
> 
> It doesn't test whether the first bind worked, and doesn't try to read 
> from a member using bind.

Hmm. That's a miss.

> * Documentation
> 
> - The examples assume a number of using directives without this being 
> stated.

Ok. A common complaint. Will fix.

> - I believe that it would be better to leave the std:: names qualified. 
> As it stands, it is not immediately obvious whether cout is std::cout or 
> phoenix::cout (a plausible assumption would be for a phoenix::cout to be 
> defined as phoenix::ref(std::cout) to avoid capture by value).

There's no phoenix::cout. It's just std::cout. So, yes, I understand
your concern.

> - The val(3) example stresses that it returns a nullary function, but 
> val(3) happily accepts (and discards) arguments - as it should.

Indeed.

> - The relationship between phoenix::ref and boost::ref is not made 
> clear, or even mentioned. Can one use boost::ref instead? In what 
> contexts? std::ref?

I guess not. I'll make sure that these are compatible. It is
possible to simply get rid of phoenix::ref in favor of boost::ref,
but I'm not sure. Thoughts?

> (As an aside, there is a slight tendency in the documentation to be a 
> bit too independent of the rest of Boost for my taste. It mentions 
> Lambda in passing, but doesn't go into any details; it completely 
> ignores boost::bind.)

Right. I'll try to fix this.

> - The "Forwarding Problem" paper is so 2002. :-)

Because the docs (from phoenix1) was originally written in 2002 :-P
I'll provide more up to date information.

> - The note that the binders are monomorphic should probably be fixed.

Yes. A historical artifact, again :-(

> - It might be instructive to add an introspection example to the final 
> chapter that returns the (minimum) arity of a Phoenix expression.

Ok.

> - The documentation is missing a reference section, or at least an index 
> of the identifiers in the phoenix namespace.

That's part of the road-map. I'll start with a quick reference and
proceed to a full reference.

> * Vote
> 
> This is hard.
> 
> On the one hand, it is ridiculous to include 
> <boost/spirit/include/phoenix_bind.hpp> for phoenix::bind instead of 
> <boost/phoenix/bind.hpp>, as one would reasonably expect. As Phoenix is 
> already in Boost, the review merely needs to formally acknowledge the 
> facts. And the library certainly meets the criteria for acceptance.
> 
> On the other hand...
> 
> I sense from the state of the documentation that Phoenix being under the 
> Spirit umbrella and not an "official top level" library has led to a 
> somewhat isolated development. It is very high quality, above the Boost 
> median, but somehow doesn't yet feel like a top level library.
> 
> Still being on the other hand...
> 
> We know that having two libraries with overlapping goals, overlapping 
> syntax, overlapping identifiers and placeholders, and subtle differences 
> is confusing and a continuous source of recurring questions. Now we will 
> have three.
> 
> But the fair thing to do is vote for acceptance. Which I will.

Thank you. Your comments and recommendations are all highly valued.

Regards,
-- 
Joel de Guzman
http://www.boostpro.com
http://spirit.sf.net