[boost] Re: [Review] Named parameters library

From: David Abrahams (dave_at_[hidden])
Date: 2004-11-13 09:16:29

"Pavel Vozenilek" <pavel_vozenilek_at_[hidden]> writes:

> "Doug Gregor" wrote:
>
>> The formal review of the Named Parameters library
>> by David Abrahams and  Daniel Wallin starts today
>>
> I vote weak YES to accept this library.
>
> - Its documentation is very weak and should
>   be expanded *significantly*.
>   More on it in note [8] bellow.
>
> - There should be more tests.
> - Problem with Intel C++ should be investigated.
>
> /Pavel
>
> _____________________________________________________
> 1. Headers should contain
>
> #if (defined _MSC_VER) && (_MSC_VER >= 1200)
> #  pragma hdrstop
> #endif
>
> on the beginning. 

Why?

> It has effect on VC and Intel C++ users.

What effect does it have?

> _____________________________________________________
> 2. named_params.hpp:
>
> Shouldn't the default for
>   BOOST_NAMED_PARAMS_MAX_ARITY
> be more than 5?
>
> Why not >= 10.

It's arbitrary.  We could do either.

> _____________________________________________________
> 3. named_params.hpp:
>
> Shouldn't the sub-namespace be named
> "named_params_detail" instead of just "detail"?

Yeah, something like that.  I guess a pass over this code to
strengthen it for Boost needs to be done.

> Names like "nil" inside look quite collision prone.
>
> _____________________________________________________
> 4. docs: the line
>
> window* w = new_window("alert2", movability); // error!
>
> should say "logical error" or "coding error" or something like that.

It's a type error.  Thanks.

> _____________________________________________________
> 5. docs: the chapter "Introduction" may be better
>    named "Problem" but YMMV.

Noted.

> _____________________________________________________
> 6. could the code snippets be visually separated
>    from the rest, e.g being in boxes and color syntax
>    highlighted?

They are already in gray boxes.  As for color syntax highlighting,
that's not easy for us to do unless we convert the documentation
source format... which is a possibility.

> _____________________________________________________
> 7. Question: is there possibility BCB could be convinced
>    to compile (part) if the library or is BCB
>    completely without any chance?

Anything is possible.  I personally won't waste any time on BCB.

> _____________________________________________________
> 8. What I miss in documentation:
>
>  - Information about overhead:
>       - numbers/graphs for at least two compilers
>       - estimate how the overhead changes with
>         # of parameters and their type

Are you curious about compile time or runtime?

>  - What happen when I change foo_impl() signature?
>    Will it quiely comoile or what kind of error
>    will I get. Is the error understandable? Example.

I am not convinced this belongs in the documentation, unless we are
going to change our documentation standards drastically.  Few other
libraries discuss the quality of error message you get when they are
misused.

>  - Few trivial and non-trivial overcommented complete examples.

Agreed.

>  - Info whether there are any additional constructors
>    or assignements called for object as parameters.

??

It seems to me that section 3 quite clearly describes how the objects
are passed.  Can you help me understand why the answer to yoiur
question wasn't clear from the text?

>  - Info about exception safety.

What, specifically, are you curious about?  There's nothing much to
say; all the library does is introduce a forwarding layer.

>  - Whether it is possible to have both "named params"
>    function foo() and the original signature function
>    "foo" and whether there could be any problems.
>    Can the original signature foo() be externs "C"?

I'm confused by the question.  Except for the need to write ref(), the
"named params" function foo supports the original calling signature.
I concede that if you have questions the docs need to be beefed up,
regardless of the questions' validity.

>  - How do the usual conversion rules apply? Exceptions,
>    examples of these, tricks to make it as safe
>    as possible.

It should be clear from the fact that the forwarding functions are
templated that no conversions apply at that point.  However, the usual
conversions do apply when the forwarding functions pass their results
on to the actual implementation function.

>  - Do overloads of "named params" functions work?
>    Example if they do.

Yes, that's the whole point of section 4, "Controlling Overload
Resolution."  Can you help me understand why that wasn't clear from
the text?

>  - How it is with exception specifications - suported,
>    unsupported, dangerous here?

Just as dangerous as ever.  Supported?  I have no idea what you
mean.  Libraries don't support exception specifications; that's done
by the language.

>  - Does "named params" allocate heap memory? 

No.

>    If so, in single block?
>
>  - Who are expected end users of the library, examples.

This is a general-purpose library.  I don't think it's fair to demand
that docs answer this question any more than you would ask who the
users of an iterator library are.  The answers are tautological:
anyone who wants to provide a named parameter interface would use it.

>  - I would welocme detailed table what features do
>    not work with this or that compiler (more than the
>    single line of text now).

On the compilers we tested, all the detail is given by the simple list
in the doc.  Would it really help to draw boxes around those lines?

>  - What is overhead of:
>      namespace {
>        boost::keyword<name_t> name;
>        boost::keyword<value_t> value;
>     }
>    being in header?

Compile-time overhead? These generate trivial objects with no data
members, constructor, or destructor.  It ought to be essentially free.

>  - Is it possible to reuse tags for different functions?

Yes, that's part of the point of the library.

>    Any problems can happen?

I suppose anything can have problems, but the library is designed to
be used this way.

>  - Does it work with Boost.Any as parameter?

I don't see why not.  Specific example, please?  Again, this seems
like an unfair question to demand that the docs answer.  We could ask
the same question about every other boost component.  Does it work
with Boost.Tuple parameters?  Yes.  etc., etc.

>  - Does it work when one used non-default
>    calling conventions as __fastcall?

Yes, and that should be obvious from the documentation.  There are no
function pointers flying around; it just uses simple forwarding.
Another unfair question to demand that the docs answer, IMO.

>  - Is it possible to export "named params"
>    function from DLL? Any problems?

You wouldn't do that.  You'd export the implementation function and
leave the forwarding function templates in the header.  But again,
this should be obvious from the documentation if you know how to write
a dynamic library that presents a templated interface.  

>  - Does it work with bind/boost.function/etc?

What do you have in mind?  Of course it "works with" any component in
Boost for some definition of "with."

>  - If I use "named params" for the same member
>    of both base and derived class: what defaults
>    would be used? Is it consistent with default
>    handling or 'normal' members?

Because the forwarding interface mechanism is completely exposed, the
answers should be obvious to anyone who knows C++, it seems to me.
These are not magical functions; they're just ordinary templates.

>  - Would it work to provide "named params"
>    overload of existing C functions?
>    E.g. Win32 API functions?

Again, that overloading works is the point of section 4.  Yes.

>  - Is it possible to use usual concept checks
>    with "named" function parameters? Will it work?

Because the forwarding interface mechanism is completely exposed, the
answers should be obvious to anyone who knows C++, it seems to me.

>  - The macros is underdocumented and never
>    used in tests.

Good point.

>  - How the library works with namespaces: can I put
>    the foo_impl into its own inner namespace? Example.

Come on, of course you can.  This is just standard C++.

>  - What does need to be in header and what can be
>    put in implementation file? Example(s).

Everything goes in the header.  You could use extern declarations for
the keyword objects and stick them in an implementation file instead
of putting them in an unnammed namespace, but it will be more
inconvenient.  Again, this is just standard C++; why is there any
mystery here?

>  - Is there way to make definition of pImpled
>    member functions easier?

Specifics, please?

>  - Iteration over parameters is undocumented.

Why should we document iteration over parameters?  Maybe I don't
understand what you have in mind.

>    No examples.
>
>  - There should be info whether the library headers
>    can be part of precompiled headers (= has any
>    compiler problems?)

It's standard C++, and subject to the same problems that any other
library might have with certain precompiled header implementations, if
any such problems exist.  I don't use PCHs, so lack the expertise to
find the specific information, but would be happy to include it in the
docs if someone else supplies it.

> - How does the library deals with std::auto_ptr
>   parameter? Could there be test for it?

It's a good question, but I think this falls again under the common
rules for C++ and auto_ptrs.  You pass ref(x) or you use the named
interface (kw = x), because auto_ptr can't be passed by const
reference.

> - typedef void (* foo_t)(const char*, float);
>   foo_t var = &foo_with_named_params;
>   Is this possible/correct/error prone/caught by compiler?

I don't see a problem.  If it compiles, it's correct.  It doesn't
make sense to me to mention all the things that just work normally in
the docs.

> - Is it possible to call function with "named params" from
>   another function with "named params" and the same signature
>   and pass these params without need to extract them?
>
>     void foo_impl(....) {
>       cout << "foo_impl called";
>       foo2(???);
>     }

Good question.  The answer is no; you have to call foo2_impl if you
want to avoid extraction.

> - Does the library work with <cstdarg>?

I'm not sure what, specifically, you have in mind, but because the
forwarding interface mechanism is completely exposed, the answer
should be obvious to anyone who knows C++, it seems to me.

> - Can or can not this library be used in a ScopeGuard
>   implementation? (I do not have exact idea how it would
>   look like.)

No offense intended, and I really appreciate your interest in the
library, but this seems typical of your questions.  The docs
shouldn't be expected to answer every vague question that pops into
your head.

> _____________________________________________________
> 9. Number of test should be much higher. These should
>    range from very simple ones to complex.

I agree.

>    All possibilities asked in [8] should be covered.

I disagree.

> _____________________________________________________
> 10. named_params.hpp: are the yes_t/no_t needed?
>     Can similar types from TypeTraits be used instead?

Probably.  I don't think it would be an advantage, though.

> _____________________________________________________
> 11. named_params.hpp: the #endif and #else
>      should have comment to what they relate.
>      The code blocks are quite large and its too easy
>      to lost track.

I have mixed feelings about that.  Those kind of comments get
out-of-date really quickly and my editor, at least, can automatically
find the matching #if.  Maybe comments should be saved for information
that can't be deduced mechanically.  Also, it's hard to know whether
the comment at the beginning and end of an else clause should match
the #if condition or should negate it.

> # endif // __EDG_VERSION__ > 300
>
> _____________________________________________________
> 12. named_params.hpp:
> #include <boost/detail/workaround.hpp>
> should be there listed explicitly.

Good catch.

> _____________________________________________________
> 13. debugging support: could there be simple function/macro
>       which iterates over all named parameters and puts them
>       (in readable, pretty form) into single string?
>      Such string can then be printed, shown in IDE tooltip
>      for examination etc.

Details, please?

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