Re: [Boost-users] [review] Fusion review

From: David Abrahams (dave_at_[hidden])
Date: 2006-05-03 19:39:26

Ronald Garcia <garcia_at_[hidden]> 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<typename Struct, int Pos>                          // 1
      struct example_struct_iterator
          : iterator_base<example_struct_iterator<Struct, Pos> >  // 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