Re: [boost] [Review] Named parameters library

From: Doug Gregor (dgregor_at_[hidden])
Date: 2004-11-22 10:39:27

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.

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

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));

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

5) I haven't yet tried controlling overload resolution, but I know I'll 
be needing this in the future if the BGL migrates to this library. 
We've used the same ideas before to integrate the Parallel BGL with the 
(sequential) BGL, where one takes the same 
algorithm---breadth_first_search, for instance---and selects the right 
overload based on the concepts the graph models (in our case, 
Distributed Graph vs. Graph generally makes the difference).

> 	- What is your evaluation of the implementation?

The implementation is very good. I did not study the details in depth, 
but the parts I looked at were well-designed. Especially appreciated is 
the ability to scale up to many more parameters (although I see that 
there was a patch required for this to work nicely).

> 	- What is your evaluation of the documentation?

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?

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.

3) Describing that operator, exists would be helpful.

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

The syntax is much improved over existing solutions. As is, the library 
is good enough that I would recommend phasing it into the Graph 
library.

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

Yes, Apple GCC 3.3, No.

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

In-depth study. I implemented the PageRank algorithm for the BGL, first 
with only the Named Parameters library under review and then, later, 
allowing both the traditional BGL named parameters and the named 
parameters from this library. I'm confident that the two named 
parameters mechanisms can coexist within the BGL. The PageRank 
algorithm, with both named parameters interfaces, is here:

        http://lists.boost.org/MailArchives/boost/msg75005.php

> 	- Are you knowledgeable about the problem domain?

Yes;  I've dealt with the BGL named parameters mechanism for quite a 
while.

        Doug