Doug Gregor <dgregor@cs.indiana.edu> writes:
My review of the named parameters library follows:
- What is your evaluation of the design?
The design is good. The library improves the syntax of named parameters greatly relative to existing solutions, makes it trivial to define new keywords, provides a simple DSL for specifying default parameters, and has some useful shortcuts.
I have a few issues with the interface:
1) We need a params.has(k) member, that returns mpl::true_ when the parameter is there and mpl::false_ when the parameter is not there. That lets us dispatch either statically or dynamically based on the existence of an argument for keyword k.
Seems like that interface makes static dispatch unneccessarily hard. How about a metafunction: has_param<Params, k> such that has_param<Params, k> is derived from either mpl::true_ or mpl::false_ and has_param<Params, k>::type is one of mpl::true_ or mpl::false_. ??
2) The macro BOOST_NAMED_PARAMS_FUN is okay when all parameters may be named parameters. In my use cases, there are always several positional-only parameters for which I do *not* want the option to rename them, and may want to be more picky about how they are passed to the function. For instance, I want to be able to take a "Graph& g" as the first parameter. One way to address this would be another variant of this macro, which I'll lamely call BOOST_NAMED_PARAMS_FUN_EXT, that might work like this:
BOOST_NAMED_PARAMS_FUN_EXT(void, page_rank, 0, 3, detail::page_rank_keywords, (2, (typename Graph, typename RankMap)), (2, ((Graph&, g), (RankMap rank))))
The first new argument is a preprocessor array giving the extra template parameters that should be introduced into the function. The second new argument is also a preprocessor array, this one containing the parameter types and names (each as tuples). I believe there is enough information here to generate, e.g.,
template<typename Graph, typename RankMap> void page_rank(Graph& g, RankMap rank) { return page_rank_with_named_params(g, rank, detail::page_rank_keywords()()); }
template<typename Graph, typename RankMap, typename T0> void page_rank(Graph& g, RankMap rank, const T0& a0) { return page_rank_with_named_params(g, rank, detail::page_rank_keywords()(a0)); }
etc...
Probably something like that could work. But my intuition tells me this is a good candidate for vertical repetition: #ifndef BOOST_PP_IS_ITERATING ... # define BOOST_NAMED_PARAMS_SETUP(whatever.hpp, 2, 5) # include BOOST_NAMED_PARAMS_GENERATE #else template <class Graph, class RankMap BOOST_NAMED_PARAMS_TYPES> void page_rank(Graph& g, RankMap rank BOOST_NAMED_PARAMS_ARGS) { return page_rank_with_named_params( g, rank, BOOST_NAMED_PARAMS_KEYWORDS); } #endif Otherwise it's just too ugly and unreadable, not to mention what happens when types contain commas.
3) Partly due to the missing "params.has(p)", we're lacking the ability to determine the type of an expression like "params[index_map | get(vertex_index, g)]" without passing it to another function. I'd gladly give up some clarity in the handling of default parameters if I could get at this information. I'd even go for an auto-like macro:
BOOST_NAMED_PARAMS_AUTO(index, index_map, get(vertex_index, g));
Yikes; that one is hard without some kind of typeof support. How about: BOOST_NAMED_PARAMS_AUTO(index, index_map, some-type-expression); ?? You already have some way to look up the type of get(vertex_index, g) right?
If we had both (2) and (3), it would be feasible to implement algorithms such as those in the graph library without manually introducing several layers of forwarding functions used only to determine types.
4) Using the name "restrict" in a public interface feels like a really bad idea given its status in C99...
Yeah, we should change that.
The documentation was a little bit lacking. There were a few specific things I would like to see:
1) A "guide for library writers" that helps one organize a library properly for named parameters. For instance, where should we put keywords? I know we should use an anonymous namespace, but should that be in namespace boost? In a "keywords" subnamespace?
Okay.
2) Keywords are stated to be of type "boost::keyword<keyword type>". Can we derive from the appropriate boost::keyword<...> instead? This is important to the BGL for backward compatibility reasons.
I honestly don't know. If we can, we should enable it.
3) Describing that operator, exists would be helpful.
Definitely. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com