[boost] Phoenix review

Subject: [boost] Phoenix review
From: Doug Gregor (dgregor_at_[hidden])
Date: 2008-09-24 16:00:40

My review of Phoenix follows. I focus mainly on the documentation,  
because I've studied the Phoenix code-base on various occasions and  
have always found it to be very clear and of very high quality. That,  
combined with its extensive use in Spirit, makes me confident in the  
implementation of this library.

I vote to accept Phoenix into Boost.

More detailed review comments follow, but my vote to accept the  
library is not contingent on any changes.

In the "Starter Kit", it would really help to state which namespaces  
(in general) the various functions like "val" and "ref" come from. I  
don't suspect that "ref" is boost::ref or C++0x's std::ref, but I  
could imagine that they could possible work with Phoenix. (As an  
aside: I bet that the references.cpp example here won't compile in C+ 
+0x, since ref() will be ambiguous).

In the "Arguments subsection, second paragraph:

        "Arguments on the other hand, evaluation to an N-ary function. An  
argument represents the Nth argument."

That's a bit confusing. Both those sentences and the example that  
follows make it look like argN always returns the last argument in the  
call, but that's not what happens. argI is an N-ary function that can  
be called with N >= I arguments and will return the Ith argument.

Also in the "Arguments" subsection, there's a parenthetical comment  
"and it's BLL counterparts: _1, _2, _3, _4, and so on). IIRC, the _#  
placeholders actually came from Boost.Bind; the Lambda library  
originally used arg1, arg2, and arg3.

In the "Lazy Functions" section, I'm wondering which protocol is being  
used for the nested "result" class template? Is it the result_of  
protocol or something else? This section needs a little more  
information, or a link, if users are going to be able to use this  
"function". You might also want to make clear that this is *not*  
std::function or boost::function, although either of those can be used  
to realize a monomorphic function from a Phoenix function.

In the "Organization" section, it would be really, really cool if that  
library-organization picture was an image map linking each box to the  
documentation on that section. (But, this is just a fluff comment)

In the "Actors" section, there's a note about the ability to set  
PHOENIX_LIMIT to something other than 10, but the word "macro" never  
shows up. Please tell the gentle reader that this is a macro.

In "Predefined Arguments", nothing says that PHOENIX_ARG_LIMIT is a  
macro.

In "Evaluating an Argument", first sentence, typo "from the those  
passed in".

In "Extra arguments", spello "desireable".

In "Composite", PHOENIX_COMPOSITE_LIMIT is certainly a macro, but the  
note doesn't say so.

Also in "Composite", are we  talking about 0..N actors or 0..N-1 actors?

In the subsection "Operation", the phrase "the containers value_type"  
should be "the container's value_type".

In the documentation for the "switch_ statement", I suggest pointing  
out explicitly that, to have multiple statements in a case_ or  
default_, the user will need to wrap the statements in an extra set of  
parentheses. The alternatively, of course, is to use a syntax like:

        case_<1>()[cout << val("one") << '\n', cerr << val("hello")],

Interestingly, catch_<exception_type>()[ ... ] uses this same syntax;  
why the discrepancy between case_ and catch_? Or did I miss something?

Anyway, now you have me trying to figure out how to tweak switch() to  
fully support break_() and fall-through from one case to another :)

In the "let" documentation, the note about PHOENIX_LOCAL_LIMIT doesn't  
say that it's a macro/preprocessor constant.

In the "Visibility" section, there's this example:

let(_a = 1)
[
     let(
         _a = 1
       , _b = _a // Ok. _a refers to the outer _a
     )
     [
         /*. body .*/
     ]
]
Personally, I would like to see this example be ill-formed. I know it  
isn't technically feasible to make bind the "_a" in "_b = _a" to the  
innermost _a, so instead I'd prefer if any use of a variable whose  
binding has changed within the same "let" would be an error. I *think*  
this can be done without too much pain through a static assertion, but  
feel free to ignore this suggestion if I'm wrong.

Is "lambda" equivalent to BLL's "unlambda" or "protect"? It might help  
BLL-savvy users to point out the relationship. Also, BLL's  
documentation points out that one should use its "protect" when  
receiving a function object as a parameter. The Phoenix documentation  
for "lambda" indicates roughly the same thing, but it could be said  
more directly. For example, a note that advises users to lambda()[]  
any function object they receive from somewhere else.

What level of interoperability can we expect among Boost.Bind,  
Phoenix, Boost.Lambda, and TR1/C++0x's bind, if any?

        - Doug