|
|
Boost : |
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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk