[boost] Re: [Review] Named parameters library

From: David Abrahams (dave_at_[hidden])
Date: 2004-11-23 14:26:00

Doug Gregor <dgregor_at_[hidden]> 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