Re: [boost] [review] Fusion review begins

From: Joel de Guzman (joel_at_[hidden])
Date: 2006-05-09 23:09:45

Eric Friedman wrote:
> 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.

You're right. Deep copying is a job for as_vector and its friends.
You are also right that this should be documented.

> 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 :)

Well, to be fair, it is not more difficult than extending MPL.
The trivial example is not so trivial after all. The example
presents a full random-access and associative sequence. In
the common case, a forward-sequence is all that's needed, and
with it, you only need to supply a few intrinsics.

But I get your point. We'll find a way to ease the burden of
extension. I am confident that we can find something nice.

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

Agreed! I like the name "keyed".

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

I retrospect, all metafunctions originally resided in the namespace
fusion::meta. Right now, there's a multitude of namespaces for different
types of metafunctions in Fusion. There's result_of, there's traits,
there's extension. Thinking back now, the original "meta" namespace
might have been the right choice after all. It does not introduce a
confusion as to what to use for what ("hey, what namespace is XXX in?").

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

Ok. Noted.

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

Noted.

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

Again, noted. I think Dan's reply is a good start towards that.

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

Indeed.

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

Understood.

> 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'm currently working on that, but I got sidetracked by some
personal matters. It would also help if the community posed some
possible *easy* uses. As far as I am concerned, my uses of Fusion
have been for rather complex libraries like Spirit and Phoenix--
not your typical day to day code. I definitely agree that we need
more "cook-book" style examples.

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

I'll let Dan comment on that.

> - example for pop_back spells it "___pop_back___" for some reason

Noted.

Thanks a lot for your review!

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