Ronald Garcia wrote:

The review of Joel de Guzman's Fusion library begins today, May 1, 2006, and continues through May 10, 2006.

:Download: http://spirit.sourceforge.net/dl_more/fusion_v2.zip

Are the docs viewable somewhere?

Ronald Garcia writes:

- What is your evaluation of the design?

It's very good overall. I find the dispatching and function selection mechanisms cumbersome and inelegant, but I'm not sure if there's a better solution. Maybe since usually iterator intrinsics have to be implemented all together, it would be best to allow them to be defined in one big class template, rather than being forced to piecemeal create a bunch of little nested templates. I'm a bit surprised to see static call() functions used instead of operator() for the implementation of a function call, and also to see the use of nested apply<> metafunctions for computing the result types of functions. I thought Joel had decided to use something compatible with boost::result_of. I'm surprised and a little concerned about what I perceive to be redundancy in the value_of metafunction and the deref_impl metafunction class. In the extension example, I see repeatedly the same patter transferring the cv-ref qualification from one type to another. Can't that be made simpler for extenders? typedef typename mpl::if_< is_const<Sequence>, std::string const&, std::string&>::type type;

- What is your evaluation of the implementation?

What I've seen is excellent.

- What is your evaluation of the documentation?

A decent start, but needs more attention. Forgive me for being too pedantic (I know that I am): * The top-level heading "Support" is misleading at best. * "As with MPL and STL iterators describe positions, and provide..." ^ move this comma-----^ ^--- to here * "A Forward Iterator traverses a Sequence allowing movement" ^^ You seem to have double spaces------------^^ where commas should be. As with most English writing, the missing comma is much less common than the extraneous one in this doc. I'm just not going to point out all the extra ones. * How does an "expression requirement" differ from a "requirement?" * IMO there's no need for novel approaches to concept documentation here [unless you are going to start using ConceptGCC notation ;-)]. The expression semantics should be an additional column in the regular requirements table. * IMO the doc formatting needs to better reflect the structure. For example, at libs/fusion/doc/html/fusion/iterators/functions.html there's no indication that the "Functions" section I'm in is a subsection of "Iterators" rather than "Sequences" Maybe we should show Fusion > Iterators > Functions above the horizontal rule on that page, for example. * On that note, the section title "Adapted" under Sequences is unacceptably non-descriptive without context. * why do sequences have "intrinsics" while iterators only have "functions?" * IMO too many of the pages At libs/fusion/doc/html/fusion/extension.html: * Is all the information in this section available in * is "ftag" a typo? Was "tag" intended? If not, what is the "f" for? * "As it is straightforward to do, we are going to provide a random access iterator in our example." Doesn't providing random access imply providing a whole bunch of iterator comparison and offsetting functionality that you wouldn't otherwise have to? Why complicate the example? Oh, you have the iterator claim it's random access, but then you refer the reader to the example. Hm. * "Notice we need both const and non const partial specialization." There's no such thing; this is confusing. Reword * it's "metafunction," not "meta function" * "A few details need explaining here: 1. The iterator is parameterized by the type it is iterating over, and the index of the current element." 2. The typedef struct_type provides the type being iterated over, and the typedef index provides the index of the current element. These will become useful later in our implementation." Aren't these points redundant with one another? What details are being explained? Just the meaning of the template parameters? The intro set me up for something a lot less self-evident. ... 5. All that remains is a constructor that initializes a reference to the example_struct being iterated over. "All that remains" doesn't seem right here, and there's plenty in the example that bears explanation but that you've left out. Maybe you should just add comments numbering the interesting lines template // 1 struct example_struct_iterator : iterator_base > // 2 { BOOST_STATIC_ASSERT(Pos >=0 && Pos < 3); // 3 typedef Struct struct_type; // 4 typedef mpl::int_<Pos> index; // 5 typedef example_struct_iterator_tag ftag; // 6 typedef random_access_traversal_tag category; // 7 example_struct_iterator(Struct& str) // 8 : struct_(str) {} Struct& struct_; }; and then your numbered list can contain simple phrases like 8. The constructor stores a reference to the struct so that when the iterator is dereferenced it can return the appropriate element. * "So how does value_of_impl get used? Well value_of is implemented as follows:" IMO this answering-your-own-question style doesn't work. We tried it in the MPL book, and took it out. Just my 2c. "template <typename Iterator> struct value_of { typedef typename extension::value_of_impl<typename Iterator::ftag>:: template apply<Iterator>::type type; };" I think you should say "get used by the library." Why don't you show the idiomatic version for metafunction classes? template <typename Iterator> struct value_of : mpl::apply_wrap< extension::value_of_impl<typename Iterator::ftag> , Iterator > {}; or maybe better, template <typename Iterator> struct value_of : extension::value_of_impl<typename Iterator::ftag> ::template apply<Iterator> {}; * "The runtime functionality used by deref is provided by the call static function of the selected MPL Metafunction Class. The actual implementation of deref_impl is slightly more complex than that of value_of_impl. We wish to return references to the element pointed to by the iterator, but we need a little bit of metaprogramming to return const references if the underlying sequence is const. We also need to implement the call function, which returns a reference to the appropriate member of the underlying sequence." This bit is hard to read. It feels like the presentation is probably in the wrong order, there's redundancy, and too much extra verbiage. But I could be wrong :) * You start using fields::name and fields::age without saying what they are or where they come from. I object to this policy: Unless otherwise noted, components are in namespace boost::fusion. For the sake of simplicity, code in this quick start implies using directives for the fusion components we will be using. because it is utterly ambiguous whether some of these functions are supposed to be called without qualification and found via ADL or not. Without warning that policy is carried forward from the QuickStart to the reference sections (which, incidentally, should be grouped as subsections of a section called Reference so the transition is clear -- this doc seems to wander in and out of formal documentation mode) so that in libs/fusion/doc/html/fusion/sequences/concepts/forward_sequence.html it is obvious that result_of is found within namespace boost::fusion... but where is begin found in begin(s)? Also I think the cover page credits me for Doug Gregor's work :)

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

Very useful.

- Did you try to use the library? With what compiler?

Yes, with several; I don't remember which exactly.

Did you have any problems?

No.

- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?

Very. I think this library should be accepted. However 1. the documentation should undergo an editorial review, to remove ambiguity, clarify the line between formal and informal, and with Strunk & White's mantra to "omit needless words" (and commas ;->) in mind. I only scratched the surface here. This is not necessarily a criticism of this particular library. All of our submissions need that. :) 2. I would like the authors to have a transition plan in place for replacement of the existing boost tuples. As long as the boost tuple library is there in its current form it will be the official tuple library of boost and I won't be able to use any fusion algorithms in my libraries because people will want to pass me boost::tuple. Making boost::tuple a valid fusion sequence would probably be sufficient. -- Dave Abrahams Boost Consulting www.boost-consulting.com

On 05/01/2006 08:49 AM, Ronald Garcia wrote:

The review of Joel de Guzman's Fusion library begins today, May 1,

In extension.htm, there's a section: Designing a suitable iterator which has code: template struct example_struct_iterator : iterator_base > { BOOST_STATIC_ASSERT(Pos >=0 && Pos < 3); typedef Struct struct_type; typedef mpl::int_<Pos> index; typedef example_struct_iterator_tag ftag; typedef random_access_traversal_tag category; example_struct_iterator(Struct& str) : struct_(str) {} Struct& struct_; }; However, there's no declaration of example_struct_iterator_tag. I assume it can just be declared (no need to define it) like the example_sequence_tag occuring just below the section titled: Enabling Tag Dispatching Actually, I think that's another typo, IOW instead of example_sequence_tag, it should be example_tag. But that makes me wonder if you didn't meand that example_struct_iterator_tag should also be example_tag. Also, since the above code is all for a specific structure, i.e. example_struct, why is there any need for including a Struct template parameter. Why not just: template<int Pos> struct example_struct_iterator : iterator_base > { BOOST_STATIC_ASSERT(Pos >=0 && Pos < 3); typedef example_struct struct_type; ... }; ?

On 05/01/2006 08:49 AM, Ronald Garcia wrote:

The review of Joel de Guzman's Fusion library begins today, May 1, 2006, and continues through May 10, 2006. The boost/fusion/iterator/equal_to.hpp has:

operator==(Iter1 const&, Iter2 const&) { return result_of::equal_to::value; } Obviously only the types are compared and not the values. Shouldn't the values be compared as well? I've got my own tuple implementation where I need to compare the types as well as values. To do this it looks like I have to specialize the above operator==. Was this intended, or is there some better way to do what I need?