Doug Gregor wrote:
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.
Thank you!
More detailed review comments follow, but my vote to accept the library is not contingent on any changes.
Snipped some parts. I'll apply the suggestions and edits after the review.
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.
Hah! I didn't know that. I've always used arg1, arg2 but I didn't steal that from lambda. Lambda became a later influence after FC++.
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".
It's the original result protocol, but it will soon change to the result_of protocol as soon as I iron out some backward compatibility issues. Eric's proto port uses the result_of protocol.
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.
Right.
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)
A very good idea. More snips here on typos and spellos and the macros. I'll apply them.
Also in "Composite", are we talking about 0..N actors or 0..N-1 actors?
Right. It's N-1. Good catch.
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.
Right!
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?
Yeah, the discrepancy... It's one of the things I'm not quite satisfied with. I do like the brackets [] but the empty parens stick out like a sore thumb to me. At one point, I used the [] for the cases, changing it later to the (). Fickle, ain't I? I'm open to suggestion. If we are to break the interface anyway, I'd follow what the community wants.
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 :)
Oh, that one! It's a tricky one to crack. Not much priority was given on that since no one really asks for it. To be honest, in terms of usability, I'd rather crack return_. I know it's doable, but it's always been a balance. I tend to shy away if the metaprogramming is getting quite involved. Not that I don't like the challenge. It's just that I want to tame down the compile times.
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.
Hmmm. Ok duly noted. Let me look into this. Yes, I think a static assertion can be easily done.
Is "lambda" equivalent to BLL's "unlambda" or "protect"?
Yes. Albeit a more powerful rendition of it.
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.
Right. Noted.
What level of interoperability can we expect among Boost.Bind, Phoenix, Boost.Lambda, and TR1/C++0x's bind, if any?
First should be the unification of the placeholders. I hate it when I have to use different placeholders for different libraries when they are essentially doing the same thing. The proto port is a good step in that direction. Beyond that, I'd like to hear from you and the community what you want to expect. It's an open road. Phoenix has already done it's job as a Spirit sidekick. It's just now that it's actively venturing outside that sphere. I would love to have you and the community plot its future. Regards, -- Joel de Guzman http://www.boostpro.com http://spirit.sf.net