Re: [boost] Phoenix v3 review

Subject: Re: [boost] Phoenix v3 review
From: Joel de Guzman (joel_at_[hidden])
Date: 2011-02-24 21:21:48

On 2/25/2011 2:07 AM, Thomas Heller wrote:
> On Thu, Feb 24, 2011 at 5:52 PM, Mathias Gaunard
> <mathias.gaunard_at_[hidden]>  wrote:
>> Here is my review of Phoenix v3.
>>
>> First, let me say that I am a big user of Phoenix v2, Spirit and Proto, and
>> that I therefore envision myself a big user of Phoenix v3 as well.
>>
>> It is a great improvement on top of Phoenix v2 (in particular, no strange
>> bugs due to broken type deduction and correct usage of result_of), and for
>> that reason alone it warrants my approval.
>> So here it is: I vote yes for inclusion.
>
> Mathias, thanks for the review and the nice words.

Thank you very much for your review, Mathias.

>> I however have concerns about its compatibility with Phoenix v2.
>> I tried to see how Spirit fared if I symlinked its underlying phoenix
>> directory to that of Phoenix v3, and it just doesn't work.
>> Mixing Phoenix v3 with Spirit doesn't work either.
>
> Yes, this is a known issue.
>
>> I believe this is a very important issue. I'm not sure it needs to be fixed
>> before the first release of Phoenix as a first-class Boost citizen, but it
>> certainly needs to be fixed ASAP by porting Spirit to use the new version.
>
> I am currently working on porting spirit to V3. I agree on all points,
> a proper switching strategy has to be developed.

Thomas, let's discuss this off-list. This is very important!

> <snipping overview>
>> Let's start on with more detailed remarks:
>> Misc
>> ----
>> Phoenix v3 is missing a phoenix.hpp header file that includes all modules,
>> which Phoenix v2 had (but in the parent directory).
>
> Nope, its not, there is the boost/phoenix/phoenix.hpp header which
> includes all of phoenix.
> IIUC it is generally frowned upon to put those headers directly in the
> boost directory.

Sure, but for consistency, we should do it anyway. Just trust the user
knows what he's doing.

>> Lazy functions
>> --------------
>> Using phoenix::function to turn a PFO into a lazy function is the most basic
>> way to extend Phoenix with new functionality, and clearly the recommended
>> way to do so according to the docs. [1] [2]
>> It is directly inherited from Phoenix v2, and only the result type deduction
>> mechanism changed.
>
> Correct. But please keep in mind. You as a user don't have to put your
> phoenix::function objects at some namespace scope.
>
>> This approach has several problems, at least in the way it is presented in
>> the documentation.
>> - global objects potentially increase binary size
>> - those objects are not PODs and therefore requires runtime initialization,
>> adding some runtime overhead at application startup
>> - instantiation happens regardless of whether the function is used or not,
>> which affects compilation time negatively.
>
> Do you have numbers for that?

:-) Mathias has a point. Let's also discuss this off-list. With Spirit,
I've already begun the migration towards an object free environment,
but that's for terminals --of which proto is known to slow down
compilation when there are lots of terminals. I am not quite sure
about function objects.

>> This method is massively used to define a whole lot of lazy functions that
>> forward to standard algorithms and container functions [3], which suggests
>> that this is indeed the recommended way to proceed.
>> It should however be avoided; prefer defining a template function that
>> constructs and applies the adapted function object, if the function object
>> can stay a POD that's better too.
>
> Do you have concrete proposal how this could look like?
> If there is no significant impact on (compile and runtime) performance
> of the points you mentioned above, it will stay as it is for now.

I agree.

>> I think it would also be valuable to add a "lazy" function (or some other
>> name) that takes a PFO and returns its lazy version, that could be used
>> inline in lambda expressions in a way similar to bind.
>
> I agree and think this would be a valuable addition.

Agreed.

>>
>> Ok, this has little to do in a review, but here it is anyway.
>>
>> I would like to have an adapter to turn a PFO into a monomorphic function
>> object (i.e. a function object with a result_type) that can be passed to
>> legacy algorithms.
>> This can be done two ways: either by giving the return type or the type of
>> the arguments explicitly.
>> This could also be used to define variant visitors with lambdas.
>>
>> I would like it if Phoenix could detect function objects that are
>> monomorphic and automatically propagate that monomorphism (but that's
>> probably hard to do).
>
> We talked about that already and I didn't forget. I think this will be
> a good addition.

Any concrete suggestions on how it would look like?

Regards,
-- 
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com