Re: [boost] [review] Boost.Fusion

From: Joel de Guzman (joel_at_[hidden])
Date: 2006-05-15 16:42:11

João Abecasis wrote:
> Hi!
> 
> This is my review of the Fusion library. Sorry for the late review...
> 
> I should probably state my bias, I've known Fusion since it was bundled 
> with Spirit and have wanted to see it in Boost ever since I started to 
> grasp its usefulness (I'll admit it wasn't right away... docs were 
> scarce back then ;-) ). I've commented on Fusion occasionally and have 
> contributed patches and code.
> 
>> Please always explicitly state in your review, whether you think the  
>> library should be accepted into Boost.
> 
> Yes, I think Fusion should be an official part of Boost.
> 
> Fusion has been in development for a couple of years and is a mature 
> library. It is unofficially part of Boost, it is available under the 
> Spirit tree. Fusion is being used by a growing number of official and 
> proposed Boost libraries. It is instrumental in the redesign of Spirit 
> and Phoenix (in queue for review?) and also in the fork and 
> generalization of Proto (out of Xpressive), as a general-purpose 
> Expression Templates library.
> 
> There's really no excuse for a library of this caliber to be relegated 
> to an implementation detail.

Thanks, Joao!

> There are a few configuration macros whose default value is hardcoded to 
> 10. They are,
> 
>      FUSION_MAX_UNPACK_ARG_SIZE
>      FUSION_MAX_VECTOR_SIZE
>      FUSION_MAX_SET_SIZE
>      FUSION_MAX_LIST_SIZE
>      FUSION_MAX_MAP_SIZE
> 
> . I'd like to see a single macro to globally set the default value for 
> all the others. Which also reminds me, all macros still need to be 
> BOOST_ified.

Right. Ok.

>> - What is your evaluation of the design?
> 
> Fusion builds on top of the design of MPL and also of the STL so I don't 
> have much to say in this regard. It is also a mature library that has 
> been in development for some time. In the past I've noticed minor 
> inconsistencies between MPL and Fusion, which I reported back (the lack 
> of unpack_args was one of them). I think these two libraries, as much as 
> possible, should be kept in sync.

Understood.

> Apart from interface exposed to library users, another important aspect 
> is the interface for extension. In Fusion-1, implementing a new iterator 
> type required one to specialize not only Fusion intrinsics, but also mpl 
> intrinsics. Things have improved since then, but I didn't look into this 
> specifically for the review. I'll try to share any comments at another time.

Yes, all fusion sequences automatically get MPL support out of the box
without any additional coding. That's one of the advantages of Fusion2.
I think this should be given emphasis in the docs.

> The algorithms also look straightforward to implement and the algorithms 
> already in the library are a testament to that.
> 
> On the directory structure used for the headers, I like the modular 
> approach but sometimes get the feeling that it's too fragmented. Having 
> more top-level reflections of individual components would ease my 
> concerns. Another aspect is that this structure diverges from what is 
> used in MPL. Given the parallel between the two libraries I think an 
> effort should be made to keep the structure similar. In fact, this is 
> also part of the user interface. Still, in the end, I recognize that 
> they're two independent libraries and must each follow its path.

Understood. I'll give this a lot of thinking. At the very least,
we'll surely have a flat set of headers that forward to the
main headers. Where to place them is a matter to deliberate on.
Maybe <boost/fusion/hdr/xxx.hpp> ? or simply <boost/fusion/xxx.hpp>
and push the main tree downwards in another directory, say, "main"?

>> - What is your evaluation of the implementation?
> 
> The implementation is very readable and even simple to grasp, once one 
> understands a few fundamental concepts. Although this aspect is not 
> required of a Boost library, I think it is great when it can be achieved.
> 
> Fusion is coded with the expectation that the library code will be 
> optimized into very tight code, which seems to be a reasonable 
> assumption with (any?) modern compilers.

I hope g++ will catch up! See Dan's post regarding some Fusion
speed tests. g++ produces the fattest and slowest code :(
I wish I was using incorrect flags or I haven't found the
best compiler switches to use for g++.

MS VC8.0 produces the tightest and fastest code, so far.
(No, I'm not a fan of MS. Those are just the facts.)

> I would like to see more compile-time concept checks and "friendlier" 
> error messages added to the implementation. Ideally, the user should 
> never see errors beyond what s/he directly uses. Unfortunately, however, 
> at places this will be at odds with the simplicity and readability of 
> the code.

Good points. I'll see how this can be addressed.

>> - What is your evaluation of the documentation?
> 
> I think I shouldn't be allowed to comment on this department... Anyway, 
> I think the docs still need some work, but I generally like the 
> direction. I'll try to offer Joel and Dan specific comments later.
> 
> For the record, and while it is no excuse, it should be noted that Joel 
> de Guzman is one of the lead developers of QuickBook (the other one 
> being Eric Niebler), which makes it harder to write the docs... I mean, 
> he can't simply complain about the tools. I'm sure Joel is taking notes 
> and, in the end, QuickBook and the rest of us will benefit as well :-)

Sure. FWIW, a lot of QuickBook improvements recently can be attributed
to Fusion documentation, in as much as QuickBook came into being when
Eric needed to document Xpressive. And further down history, QuickBook
had a former life as QuickDoc, which I wrote as a Spirit example and
later, as a doc tool for Phoenix. Often times, one has to write tools
to write tools to write tools to write tools...

>> - What is your evaluation of the potential usefulness of the library?
> 
> I believe Fusion has great potential as a library building block but 
> also as a more application-oriented library. On the library building 
> block side, I believe it has the potential to become a fundamental/core 
> piece of Boost. For application developers, Fusion takes tuples to a new 
> level, allowing one to make the most of information that is available at 
> compile-time.
> 
>> - Did you try to use the library? With what compiler?
> 
> Yes. Mostly with gcc (3.4 and over), also with intel-9.0 (IIRC), both 
> under linux.
> 
>>   Did you have any problems?
> 
> No. Joel and Dan have been very helpful and supportive for any doubts 
> and issues I have had and given the readability of the code I have been 
> known to provide fixes to typos and other minor issues.
> 
>> - How much effort did you put into your evaluation? A glance? A
>>   quick reading? In-depth study?
> 
> I suppose it qualifies as an in-depth study for the code and a quick 
> reading for the docs. I've followed Fusion evolution since version 1 and 
> have perused the code thoroughly. I implemented unpack_args for fusion-1 
> for my own purposes and later ported and contributed it to fusion-2.
> 
> As a user of the library, I've privately used Fusion to re-implement and 
> extend Spirit's closures, to allow members to selected and aggregated 
> for return. Something like,
> 
>      closure<int, return_<float>, return_<double> >;
> 
> , in the context of Spirit, would make a parser return a Fusion.Sequence 
> of <float, double>. If also used Fusion in other pet projects.
> 
>> - Are you knowledgeable about the problem domain?
> 
> I'm not sure I can categorize Fusion into a single and specific problem 
> domain. I think the potential use cases are vast and the domain too 
> broad. I can't claim to be knowledgeable in all the possibilities it allows.
> 
> 
> I'd like to congratulate the developers of the library on the great 
> library and thank them for exploring the concepts and bringing it to us! 
> I really expect to see Fusion as an official Boost library, soon ;-)

Thank you very much!

Regards,
-- 
Joel de Guzman
http://www.boost-consulting.com
http://spirit.sf.net