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.
> 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.
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.
> 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'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
Thanks a lot for your review!
-- Joel de Guzman http://www.boost-consulting.com http://spirit.sf.net
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk