Re: [boost] Phoenix v3 review

Subject: Re: [boost] Phoenix v3 review
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2011-02-24 13:07:04

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.

> 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.

<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.

> 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?

> 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 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.

<snip>

> Phoenix as a Proto-based library
> --------------------------------
>
> Extension of Phoenix is very nice: you can plug custom subgrammars and
> subnodes non-intrusively, making it very modular.
>
> Phoenix expressions are also refinements of Proto expressions, so I can use
> Proto transforms to alter the tree.
>
> It is also possible to specify custom evaluation strategies on a per-node
> basis, which paves the way for many exotic applications.
>
> Terminals can also be customized for evaluation within the default strategy.
> I'm not entirely sure this feature is not redundant, but I'm not familiar
> enough with the code to tell.

It is not redundant. The thing is that proto only recognizes one tag
for a terminal expression. To distinguish between all those different
terminal types, we needed the custom terminal customization point.

> Phoenix as Proto with statements
> --------------------------------
>
> One thing I was hoping Phoenix v3 would be is Proto extended to support
> statements.

I don't understand, what do you mean by that?

> While it comes pretty for some uses, it still fails one one point: the
> ability to define custom languages that embed Phoenix statements.

The ability is there ...

> The missing piece is handling of domains (extends are missing too, but I
> don't think they make sense at the statement level).

... and you don't need necessarily need the proto domain feature for that.

> I think that area needs some research though, so it's not relevant to
> acceptance at all.

It indeed needs a lot more thought.

> Documentation
> -------------
>
> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/modules/bind.html>
> says that bind is monomorphic.
> I thought it was polymorphic now?

It is, good catch, it is a relict from old times.

> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/actor.html>
> doesn't talk about perfect forwarding.

It is mentioned here:
http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/inside/actor.html#phoenix.inside.actor.function_call_operators

> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/inside/actor.html>
> Actor cannot be both a concept and a model, it's a concept and a refinement.

Thanks. This is corrected.

> It would also be nice to highlight the difference with the PFO concept.
The difference is that it is a proto expression template wrapper.

> "The problem is that given an arbitrary function F, using current C++
> language rules, one cannot create a forwarding function FF that
> transparently assumes the arguments of F. "
> That's wrong. It's not impossible, it just requires an exponential number of
> overloads. C++0x rvalue references allow to reduce this to a linear amount.

We can only *emulate* perfect forwarding. THe sentence you claim is
wrong is just a reformulation of the "Perfect Forwarding Problem"
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1385.htm)
problem statement.

> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/inside/expression.html>
> spurious ] at the end.
> The layout of this section is weird, by the way. The macros appear on top
> but are not explained until a later page.

This is because of quickbook, the macros are subsections of this
section, and the TOC is created for them ... I will try to find a way
that make it less confusing.

> I remember I saw a couple other typos and similar but I can't find them
> anymore. Anyway it needs some proofreading.

I agree :)

> Features I would like to see in a future version
> -------------------------------------------------
>
> 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.

> I would like it if the polymorphic function objects generated by Phoenix
> were masked out using SFINAE if their body-expression would result in a hard
> error.
> Essentially, to do this, one needs to be able to test whether the default
> Proto transform of an expression leads to an error. While this should be
> possible with compilers supporting extended SFINAE, I haven't been able to
> grok the Proto internals well enough to do it yet myself.

Yes, I was thinking about that lately too! Maybe we can work out an
implementation of that feature together!