Re: [boost] [Review] Fusion

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

Douglas Gregor wrote:
> My review of the Fusion library follows, but I'll spoil the ending  
> now: I vote to accept Fusion into Boost.
> 
> I was very interested to look into this library. I've found  
> heterogeneous sequences to be extremely interesting and, in some  
> cases, precisely the right answer. I've had to write about 1/3 to 1/2  
> of the heterogeneous algorithms provided by Fusion at various times,  
> time which could have been saved and code that could have been  
> cleaner if Fusion was around. So, I'm quite familiar with  
> heterogeneous containers and algorithms.

And I should give credit where credit is due. I saw the first
implementations of tuple algorithms from you. Thank you too for
sharing your early insights when Fusion was first being developed!

That other 1/2 to 3/4 seems interesting. Would you happen to have
some more algorithms that we can add to Fusion? Also, being
intimately familiar with them, would you also happen to have
some nice, easy and practical examples we can use? I must
admit those are sorely lacking.

> I was not able to review the library in-depth, unfortunately, but I'm  
> itching to get my fingers on it when time allows. So, what follows is  
> really just a bunch of minor nits from reading the documentation.
> 
> Overall:
>    - I'm not a fan of naming libraries anything but what they do.  
> "Fusion" makes sense when you understand the context, but nobody  
> searching through the list of libraries will see "Fusion" and think  
> "AHA! That's what I need". Alas, I don't have a name in mind.

"Fusion" was supposed to be just a code name. I too can't think of
any reasonable name. So, the name stuck. I'm all ears for "standard"
sounding name. At the very least, I can probably keep the code name
as a subtitle. For example: Boost Tuples Library "Fusion". Alas,
that name is already taken.

> Description:
>    - Spelling error: "hetrogenous"

Oops

> Introduction:
>    - Parts of the introduction are a bit too colloquial for my taste,
>    in particular, the paragraph.
> 
>      "Hmmm, kinda reminds us of STL right? Right! Can you imagine how it
>      would be like if you used STL without the algorithms? Everyone
>      will have to reinvent their own algorithm wheels."

Ok. How about if we separate the tutorial from the reference manual
proper as others have suggested? That way, we can keep the tutorial
somewhat colloquial and present a formal reference manual after.

>    I'd rather see them written in a more direct manner. Not that I
>    prefer boring, but at some point the whimsical writing trivializes
>    the library.

Understood.

> Top of the Iceberg:
>    - In the [1] footnote, spelling error: "Orgainization"

Oops2

> Support:
> 
>    tag_of: I believe I understand the usefulness of this trait, but
>    only because I understand tag dispatching already. Could the
>    description be expanded a little, to at least say that tags uniquely
>    identify the kind of sequence or iterator, e.g., list, map, etc.?

That's definitely an improvement. Actually, I wouldn't mind naming
them: sequence_identity<S> and iterator_identity<I>, to better suit
their intent.

>    category_of: I'm not keen on using the same trait (category_of) for
>    two different kinds of entities (sequences and iterators). Traits
>    classes should be tied to a particular concept, so I'd much rather
>    see separate "iterator_category" and "sequence_category"
>    metafunctions.

Ditto. Ok.

>    pair: I tripped over the name of this entity a bit. It's a little
>    odd to have a "pair" with only one kind of data. I guess I'd feel
>    better if this were called "keyed_value" or "key_value", because
>    it's easier for me to copy with a typed key than it us for me to
>    have a pair that's very different from std::pair.

Understood.

> Iterators:
> 
>    distance: Is the return type of distance(i, j) "int" or "convertible
>    to int"? I could imagine that in many cases it could be an
>    integral_constant<int, N>...

That should be convertible to int. Yes, you are right. It is an
mpl::integral_c<int, N>.

> It looks like Joel and Dan did a great job.

Thanks!

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