[boost] Re: [Review] Named parameters library

From: David Abrahams (dave_at_[hidden])
Date: 2004-11-17 18:41:25

Cromwell Enage <sponage_at_[hidden]> writes:

> Huzzah, my first review!
>
>> - What is your evaluation of the design?
>
> Groovy, but not quite the schiznit.

Wow, and the first review I've seen that had real street cred.

> 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?  

Yes.

> Since they may be used so often,
> should they be defined in a single library-specific
> header, e.g. <boost/graph/keywords.hpp>?  

Not neccessarily.  They should probably be decoupled and included as
needed in each header.

> And would it
> help if each name had an appropriate prefix, like
> "bgl_", to help avoid object collisions?

Not much; I would nest the unnamed namespace inside boost (or a
sub-namespace thereof).

> 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.

I don't understand.  Why would you need to be able to get the type in
order to do that?

  p[dependent | some_expression.involving(p[dependency])]

Nevermind, I think I understand now:

  p[dependency | default1]
  p[dependent | some_expression.involving(p[dependency | default1])]

I agree that you ought to  that capability.

> May I safely assume that we plan on replacing the
> bgl_named_params mechanism with this one in the
> not-too-distant future?  

That's really up to the BGL maintainers, but last I heard yes.

> 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=.

I'm not positively sure what you're saying here.  Our design does use
operator| for "providing" defaults.  They're never "assigned."  To do
that, you would (tautologically) have to use 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.

Thanks, I'm glad you tracked that down.  We need to figure out what to
do about it.

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

Hey, nifty!

>> - 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.

Yeah, we probably ought to document the operator, usage up front, too.

>> 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.

I'm positive we can do something about it without too much trouble.

-- 
Dave Abrahams
Boost Consulting
http://www.boost-consulting.com