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
In "Evaluating an Argument", first sentence, typo "from the those
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)
_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?
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk