Re: [boost] [Review] Named parameters library

From: Cromwell Enage (sponage_at_[hidden])
Date: 2004-11-17 04:05:06

Huzzah, my first review!

> - What is your evaluation of the design?

Groovy, but not quite the schiznit.

A few major questions that will pop up include how
users should name the keyword objects for their
functions, and where these should be declared.  Take
the headers-only BGL as an example: should the keyword
objects be in an unnamed namespace, according to the
named-params docs?  Since they may be used so often,
should they be defined in a single library-specific
header, e.g. <boost/graph/keywords.hpp>?  And would it
help if each name had an appropriate prefix, like
"bgl_", to help avoid object collisions?

Also, I second the feature request of getting the type
of a named parameter.  Numerous BGL algorithms require
a graph, an index map that defaults to
get(vertex_index_t(), graph), and a
color/rank/distance map that defaults to a
random_access_iterator_property_map over the index
map.  Without the type-getting ability (and thus the
ability to make temporary assignments), I may have to
duplicate the default-assigning code over several
places, like I'm doing now retooling my graph
algorithms in the sandbox.  What I'm really asking for
is an elegant way to assign a parameter a default
value that is dependent on another parameter.

May I safely assume that we plan on replacing the
bgl_named_params mechanism with this one in the
not-too-distant future?  If so, then I feel that these
issues need to be addressed.

> - What is your evaluation of the implementation?

Mmmm, preprocessing and MPL, like spaghetti and
meatballs.  So difficult to cook up, but so delicious
when served.

On the plus side, it beats having to use the
undocumented choose_pmap and choose_const_pmap
functions to extract property maps from
bgl_named_params, among other things.

I find operator| to be the most logical operator for
assigning defaults, more so than operator& or
operator=.

The major stumbling block concerns
BOOST_NAMED_PARAMS_MAX_ARITY.  It looks like it is set
arbitrarily to 5, but setting it any higher (before
including any header files, as suggested) led to
errors like "no such type named boost::mpl::apply9". 
This meant that BOOST_NAMED_PARAMS_MAX_ARITY is
actually tied to something in the MPL library, namely
BOOST_MPL_LIMIT_METAFUNCTION_ARITY.  IIRC in addtion
declaring *that* value,
BOOST_MPL_CFG_NO_PREPROCESSED_HEADERS has to be set
before including a library header.  I doubt an
ordinary user would relish following these steps just
to pass in more than 5 arguments.  And many of the BGL
algorithms take in more than 5 arguments.

I believe the modified <boost/named_params.hpp> header
attached to this message solves this problem, but I'm
not sure.

> - What is your evaluation of the documentation?

The autogenerating macro should have a more prominent
place in the docs.  Potential users might get turned
off by the number of function overloads they see
first, before they get to see the usefulness of this
macro.

I believe the "best practices" section that someone
else came up with (or was that my imagination?) should
include suggested ways to resolve the keyword object
issues I raised.

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

It would definitely be useful even outside the BGL. 
In my multistate mazes project, for example, I have a
Parallelepiped class template whose constructor and
initializing member function take in something akin to
a grid-bag layout (i.e. cell-padding, cell-spacing,
and cell-size parameters).  This named-parameters
mechanism might just supplant the monolithic class
template I'm currently using to represent this layout.
 That my multistate mazes project also uses the BGL is
somewhat beside the point...

> - Did you try to use the library?  With what
> compiler?  Did you have any problems?

I used my main development compiler, GCC 3.2 (MinGW,
Dev-C++ 4.9.8.5).  The docs state it as a supported
compiler, but the test program did not compile until
Daniel Wallin's workaround was applied.  Oh, well.

> - How much effort did you put into your evaluation?
> A glance? A quick reading? In-depth study?

My effort is in-depth, but not as much as I'd like. 
(I have yet to test any function that might require
boost::ref(), for instance).

> - Are you knowledgeable about the problem domain?

I'm familiar with the principle of named parameters,
but not with many of the technical challenges.

> As always, please remember to clearly state whether
> you believe the library should be accepted into
> Boost.

The resolution of the design issue concerning
parameter-dependent defaults bears more weight on my
vote than any of the other pluses or minuses I've
mentioned so far.  If it can be resolved easily, then
I vote yes.  Otherwise, not until it is resolved.

                              Cromwell Enage

                
__________________________________ 
Do you Yahoo!? 
The all-new My Yahoo! - Get yours free! 
http://my.yahoo.com