On 2/25/2011 2:07 AM, Thomas Heller wrote:
On Thu, Feb 24, 2011 at 5:52 PM, Mathias Gaunard <mathias.gaunard@ens-lyon.org> 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