Re: [boost] Phoenix review

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

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