Cromwell Enage <sponage@yahoo.com> 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