Re: [boost] [review] Fusion review begins

From: Eric Friedman (ebf_at_[hidden])
Date: 2006-05-09 21:01:53

My review of the proposed Boost.Fusion library follows. In general, I 
think the library will be useful, at least for library developers. My 
time for the review was very limited, and so I have not actually spent 
any significant amount of time using the library. Hopefully my feedback 
will be useful nonetheless.

 > - What is your evaluation of the design?

I appreciate the concern about efficiency and the consequent use of 
laziness in constructing views. But since the underlying sequence is 
held by reference, a view cannot outlive the underlying sequence. What 
is the solution if this is in fact desired? Is the solution to use 
as_vector or one of its kin on the resulting lazy view? There is no 
discussion or acknowledgment of this in the documentation as far as I saw.

The extensibility feature of the library is potentially very useful. 
That several foreign libraries -- MPL, std::pair, etc. -- are supported 
as first-class Fusion sequences without modification to those libraries 
is compelling. From my brief glance at the implementation, the 
extensibility feature also seems to simplify the support of the several 
Fusion sequence types. However, it seems that there is significant 
overhead in taking advantage of Fusion's extensibility. For major 
existing components such as std::pair, this overhead can be taken care 
of once and included with Fusion. But for an end-user of the library the 
story is far more complicated. In particular, the amount of effort 
required to support even the trivial example_struct indicates to me that 
very few user types will ever have Fusion support in this manner. Dave 
Abrahams made a similar comment, but I think this more thought is needed 
here. If I had more time for this review, I would have done some of the 
thinking myself :)

With regard to naming, I think fusion::pair is a bad name. Perhaps it 
should be instead named fusion::keyed? I think the name pair should be 
reserved for two values (std::pair) or two types (mpl::pair). The mixing 
of the two was unexpected.

I find the metafunctions in the library not as straightforward as they 
should be. On the one hand, it seems nearly all of the metafunctions are 
the same in functionality and use, simply providing the result type of 
the various functions in the library. Then there is something like 
fusion::result_of::value_of that does not correspond to any function. 
Why? Of what function exactly is this the result? I think metafunctions 
like this should be in the plain fusion namespace.

Aren't the arguments to the function object called by the fold algorithm 
"backwards?" That is, to follow the MPL convention, it would be (state, 
elem) not (elem, state). Maybe other functional languages do it 
differently? If so, I am not sure where it is better to have 
consistency, though I would lean towards consistency with MPL.

I was going to suggest an "apply" algorithm, which would invoke a 
function with the elements of a sequence as arguments. From reading the 
newsgroup, I see this comes in the form of unpack_args. As noted 
elsewhere, this should be documented.

 > - What is your evaluation of the implementation?

I did not look at it extensively.

 > - What is your evaluation of the documentation?

There needs to be more discussion of where Fusion is appropriate or not 
appropriate. I see there has been discussion about this on the newsgroup 
recently. This sort of discussion should appear in the documentation as 
well. Thinking about how to explain this may also lead to a good simple 
example to demonstrate a case where the library would be useful.

For instance, I initially wondered why there is not a filter algorithm 
that takes a runtime predicate. The answer, of course, is that this 
would not be possible, since the length of the resultant sequence would 
not be known at compile-time. Though this is obvious upon even the most 
basic reflection, it was not immediately obvious to me. Documentation 
could have helped here.

There should be discussion of the approach Fusion takes with respect to 
laziness and holding the underlying views by reference. The performance 
claim made here is probably, though maybe not, obvious. More important, 
though, and also not discussed is how to make a deep copy of a view, in 
case it needs to outlive its underlying sequence.

Not to unnecessarily belabor the point, but the documentation badly 
needs examples other than how to extend the library. I understand the 
problem of coming up with a simple (but also non-trivial) example is not 
easy. Even if the example is somewhat involved or complex, ddiscussing 
it in the documentation would be incredibly useful to people such as 
myself approaching the library for the first time. As for the audience 
of such a discussion, I think you can assume your readers are familiar 
with metaprogramming and other advanced C++ techniques. What I want is a 
discussion that shows me how Fusion can be so useful -- not merely 
leaves me with the sense that it probably is, kinda, maybe :)

I think the documentation as a whole needs some reorganization. If 
nothing else, I think the documentation for the metafunctions should 
appear in a separate section from the regular functions. As it is, the 
metafunctions appear quite predictable, mostly for determining return 
types, and so the metafunction reference disrupts the flow of reading 
the documentation. On a related note, if there is in fact anything 
unusual or special about some of the metafunctions (apart from my 
comment above about value_of, etc.), I missed it in the monotony of 
their documentation.

I also have a few minor complaints about typos, etc. in the 
documentation, which I note below.

 > - What is your evaluation of the potential usefulness of the library?

Since my time with the library was limited, I am left only with an 
impression. My impression is that it is potentially very useful in the 
development of libraries.

 > - Are you knowledgeable about the problem domain?

Somewhat. I used MPL quite a bit in the development of Boost.Variant, 
and I have used metaprogramming techniques in other projects, for 
serialization/deserialization of data to pre-established wire formats. 
Particularly in the latter project, I think Fusion's facilities would 
have been helpful.

--
Minor documentation issues:
- example for zip algorithm has syntax "make_vector((1,2),('a','b'))" -- 
not real, is it?
- example for pop_back spells it "___pop_back___" for some reason