Re: [boost] Review: Function library

From: Douglas Gregor (gregod_at_[hidden])
Date: 2001-06-13 09:21:03

On Wednesday 13 June 2001 08:32, you wrote:
> I have some quibbles with the design, but I've found this sort of
> callback library awfully useful and the code looks fundamentally
> sound so I think it should be accepted into boost. I tested it with
> CodeWarrior 6.1 on the Mac and the only problems I ran into were some
> niggling little details because I had the warnings cranked up (see
> below).
>
> There are a few design decisions that I'm dubious about:
>
> 1) I don't see why policies and mixins should be built into function.
> I think the way to handle this is sort of like the way argument
> binding is handled: clients would call a binder function with the
> function and the pre/post functions and a boost::function would be
> returned that points to an appropriate function object.

Policies and mixins are built-in so that they can be an attribute of the 
bridge between a boost::function object and whatever it calls. The example 
that motivated the design is synchronization: mixin a class containing a 
mutex, then the precall waits for the mutex and the postcall signals the 
mutex (I posted pseudocode earlier). Here the burden of synchronization is on 
the code that declares the boost.funciton object; using wrappers, the burden 
is shifted to the code that assigns values to the boost.function. There are 
many more points of failure with the wrapper approach.

> 2) I'd prefer it if calling a function with an empty target was
> treated as a precondition violation (and handled with an assert). The
> rule of thumb that most people seemed to buy into was that asserts
> should be used where clients can easily preflight and exceptions
> otherwise...

I think I agree with you on this one... unless there are dissenters with good 
points to make, I'll go with the assert.

> 3) In the absence of a bind library it seems silly to force everyone
> to generate their own copies of the bind code in the FAQ. I would
> imagine that nearly everyone who uses boost::function is going to
> need to work with methods so I think their should be a stopgap
> solution inside boost.

Anybody working on this? If not, I'll come up with something simple.

> I added some new tests to function_test.cpp. At the top I added a
> couple of new functions:
>
> static int do_stuff(int n = 0)	{(void) n; return 0;}
> static int write_return_three() { global_int = 3; return 3;}
>
> And in test_zero_args() I added some code:
>
>    // Use default argument
>    function<int> d(do_stuff);
>    BOOST_TEST(d() == 0);

I stuck these into a separate testcase, defarg_test.cpp, because some 
compilers were failing.

>    // Test compatible return types
>    function<long> l(generate_five);
>    BOOST_TEST(l() == 5);

Added.

>    // Ignore return values
>    function<void> i(write_return_three);
>    global_int = 0;
>    i();
>    BOOST_TEST(global_int == 3);

Already in function_test.cpp: write_three_obj has an 'int' return value which 
is swalled by the boost::function<void> objects.
 
>    // Test const function's
>    const function<int> c(generate_five);
>    BOOST_TEST(c() == 5);

Already in function_test.cpp: write_const_1_nonconst_2 ensures that 
boost.function differentiates properly between const and non-const.

> Here are the changes I had to make to get the code past the compiler:
>
> 1) There are signed/unsigned conversion problems in
> allocator_test.cpp: I changed the allocate and deallocate methods so
> they take a std::size_t instead of an int.

OK.

> 2) In catch_exceptions.hpp I made report_exception() inline (static
> would work just as well).
>
> 3) In defarg_test.cpp I made test_two_args() static.

OK. 

> 4) In function_test.cpp I made test_zero_args(), test_one_arg(),
> test_two_args(), and test_emptiness() static.
>
> 5) In test_main.cpp I made report_error() and report_critical_error()
> static.

OK.

> 6) I undef'ed nil at the top of function_base.hpp, but as was
> mentioned this isn't going to work in the real world.
>
>    -- Jesse

It appears that 'nil' will become 'null' (which doesn't seem to have the same 
problems) and will likely not even exist in the first release...

Thanks for the comments.

        Doug