Re: [boost] More comments on the Bind library submission?

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2001-08-18 04:10:47

Here are my review comments for the proposed bind library.

In general, I'd like to see the library accepted on the
condition that reference documentation be added and the mem_fun
trouble be cleaned up (see below).

Tests
-----

 - I'd like to see the tests merged into one .cpp file so there's
only one more line in the tables for the regression test results.
Also, please consider using Boost.Test (possibly even for each
function call.)

 - como 4.2.45.2, gcc 2.95.3, and gcc 3.0 didn't have any problems
compiling the library.

Documentation
-------------

 - The HTML pages should set the background color to "white"
explicitly.

 - As pointed out elsewhere, return_type needs to be changed
to result_type (also in the documentation).

 - The "purpose" section in the documentation only talks
about "function objects, function pointers, member function
pointers".  However, the examples just below show how
functions (or references to functions) are used, not pointers
to such (i.e. there is no &f or &g).  This is inconsistent.

 - This:
>>> bind(f, 5, _2)(x);                     // f(5, x)
should replace _2 by _1.

 - "Using bind with member function pointers":

 bind(&X::f, ref(x), _1)(i);        // x.f(i)
 bind(&X::f, &x, _1)(i);            // (&x)->f(i)
 bind(&X::f, x, _1)(i);
 bind(&X::f, p, _1)(i);

First, the last two lines should also have a comment indicating the
semantics. I presume it's  "x.f(i)"  and "p->f(i)".
I find these semantics somewhat confusing.  It appears that the mem_fun
or bind stuff is specialized for "T*" and "smart_ptr<T>", because
it appears to use "->" in these cases and "." in all the others.
This needs more explanation, I believe.  Or a reference to the
mem_fun documentation, where it's described.

 - "MSVC specific ..."
I'd like to see the BOOST_BIND ugly hack strongly discouraged and
the BOOST_BIND macro renamed to something more detesting, e.g.
BOOST_MSVC_BIND_NAME_CLASH_AVOIDANCE (please find a better name).

 - The visitor explanation is seriously lacking.  What's the
"internal state" of a bind function object?  What requirements
are imposed upon "v"?  Yes, the visitor pattern is supposed
to be an implementation detail, but on the other hand, it's
supposed to be used by other libraries, so it's indeed an
exported feature.

 - I like the general introduction, but I'm missing the
formal reference documentation after the synopsis.  What's
the precise semantics of bind?   Can it throw exceptions
(possibly by allocating dynamic memory)?
Are there any additional preconditions to be met by the
arguments of bind?  What's the interface contract of the
function objects returned by bind?

 - For the less ingenious reader, the introduction should
contain at least one example where the result of bind isn't
directly used to apply operator().  Otherwise, some readers
may think "why isn't he simply calling the function without
the funny bind stuff in between".  For example, use
std::for_each to show how bind can be useful.

 - The library uses the names _1, _2, ...  I don't like
that very much, but I can live with it.  However, I'd like
to see the definition of _1, _2, ... factored into a
separate "detail" header so that a future lambda library
can (hopefully) re-use the definition.

mem_fun:
 - There's a clash with Boost.Functional.  I think that
Boost.Functional's mem_fun has a slightly different purpose
than bind's mem_fun: The former wants to fix up a few
nuisances of std::mem_fun, while the latter tries to
generalize it a lot more.  Since the standard and
Boost.Functional was there first, bind's mem_fun must
change its name.  What about member_fun?  After all,
when using bind, this appears not to be user-visible.

I strongly oppose any include-order dependency hacks
suggested previously.

 - The get_pointer() approach requires additional
co-operation from smart pointer classes not required
when using std::mem_fun_ptr.  Therefore, bind's mem_fun
is not a fully backwards-compatible substitute for
Boost.Functional's mem_fun_ptr (or std::mem_fun_ptr, for
that matter).   Since bind uses bind's mem_fun implicitly,
I'd like to see a note in bind's documentation.

 - The documentation is missing a reference section.

ref/cref:
 - This has the beginnings of a reference section, but I'd
like to see it more clearly structured (similar to what
is now in the C++ standard, i.e. have separate lines for
a function which say "Throws:", "Returns:", or "Effects:".

Implementation
--------------

mem_fun.hpp:
 - Line 35:  I believe a forward declaration of
boost::shared_ptr<> is insufficient here, because the
get_pointer() implementation calls p.get(), which requires
a completely-defined type.

 - I don't like the _mfi and _bi namespaces.  Boost
uses the "detail" namespace for implementation details.  I hope
that's also stated in the library guidelines somewhere.

 - After initial publication, usefulness of Boost.Preprocessor
as applied to this library should be evaluated.

bind.hpp:
 - Could the implementation use the tuple library?

Jens Maurer