[boost] Phoenix review

Subject: [boost] Phoenix review
From: Peter Dimov (pdimov_at_[hidden])
Date: 2008-09-29 20:38:45

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.

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

- 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.

- 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.

- bind<R> is not supported.

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

- What is the equivalent of boost::unlambda in Phoenix? 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 )();

- 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.

* Implementation

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

- 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.

- 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.

* Documentation

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

- 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).

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

- 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?

(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.)

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

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

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

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

* 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.