Re: [boost] Review: Function library

From: Douglas Gregor (gregod_at_[hidden])
Date: 2001-06-07 19:36:49

Darin,
        Thank you for your comments. I'll intersperse my responses throughout...
        
On Thursday 07 June 2001 06:55 pm, you wrote:
> My apologies for not reviewing this library more carefully before the
> formal review stage. I have good excuses, but I won't bore you with them.
> Here are my comments:

        
> --------
>
> I found it difficult to understand the function library given the
> documentation provided. In particular, I am accustomed to the libsigc++
> library, which excels at abstracting the differences between member and
> non-member functions in its signal handlers (callbacks, functions,
> whatever). This library takes a different approach to solving a similar
> problem embracing function objects rather than arbitrary member functions,
>   which does seem better.
>
> --------
>
> I don't think the documentation provides sufficient motivation for why
> boost::function objects exist. It doesn't clarify how they differ from
> existing function objects or function pointers. After digging, I was able
> to find these reasons:
>
> 	1) non-template code can work with arbitrary function pointers or
> object, rather than requiring that the type of the function pointer or
> object be a template parameter (this seems to be the big feature)
> 	2) a variable of a single type can hold either a function object or a
> function pointer
> 	3) functions with different parameter or return value types that are
> compatible can be used, even if the types are not exactly the same
> 	4) starts out empty rather than uninitialized
> 	5) gives predictable behavior (exception) when you call through a null
> pointer rather than undefined behavior
> 	6) explicit empty(), set(), and clear()
> 	7) == and != fail to compile or link rather than giving useless results
> 	8) can mix in a base class so there can be state in the
> boost::function container itself
> 	9) can attach precall and postcall with Policy so the boost::function
> container itself can do additional work

Such a list would be very useful indeed in a paragraph/section describing the 
reasons to use a 'function' object.

> And a few reasons raw function pointers or function objects can be better
> for some things:
>
> 	1) smaller
> 	2) faster
> 	3) more inlining possible
> 	4) better error messages from most compilers when used incorrectly
> 	5) (for function pointers) = 0, == 0, and != 0 work as with other
> fundamental types
> --------
>
> The sentence in the documentation that says "Free function pointers can be
> considered singleton function objects with const function call operators"
> doesn't make it clear that treating function pointers in this way is a
> feature of the library, not something intrinsic to the C++ language.
> Instead you could say that "The library treats [...]".

I believe this to be a feature of the language, and not of the library. A 
function object can be copied, default-constructed, executed, etc. and may 
have internal state. A function pointer fulfills all of these requirements 
except that it is a singleton (only one copy of the function's internal data 
for all instances) and is const because it has no instance variables that it 
could modify.

The intention of the sentence in the documentation is to try to categorize 
the relationships between function objects and function pointers. Some would 
say that a function object is just a restricted function pointer. However, I 
prefer to view the function object concept as the basic notion of being 
invocable and the function pointer is merely a model of the function object 
concept.

> --------
>
> I think that function<>::operator= should be implemented with a call to
> this->set() for simplicity.
>

I didn't do this only because it would cost (yet another) copy of the 
incoming function object.

> --------
>
> The source code contains 8-character tabs. I think that most Boost code
> just uses all spaces for indentation, avoiding tabs, and that's what I'd
> prefer.

Will do.

> --------
>
> BOOST_FUNCTION_FUNCTION::operator const detail::undeletable* () const
> returns reinterpret_cast<const detail::undeletable*>(this) to indicate
> true. A better approach would be to define a global object and return a
> pointer to that, perhaps called non_nil. The assumption that a
> reinterpret_cast will not yield a null pointer in this case is probably
> correct in practice, but it seems straightforward to change things so we
> don't have to rely on this, and I don't think it's guaranteed by the
> standard.

I believe what I'm doing is safe, and here's why: looking at the C++ 
Standard, 5.2.10/7 says I can convert between object pointer types and the 
result of, for instance,
A* p = /* ... */;
reinterpret_cast<A*>(reinterpret_cast<B*>(p));

will be equivalent to p. Also, 5.2.10/8 says that the null pointer value of 
one type is converted to the null pointer value of the other type. So if a 
pointer of type A mapped to NULL of type B, casting back to A would give NULL 
- therefore the pointer to A was NULL in the first place.

> --------
>
> The comment at the end of namespace intimate says "// end namespace
> private".

Ok.

> --------
>
> The documentation could use a list of everything that's defined by the
> function library to help readers understand the overview. Here are some
> things defined in the boost namespace, but not mentioned in the
> documentation:
>
> 	nil_t
> 	nil
> 	make_function
> 	function_traits_builder

Those definitely deserve a mention (and make_function needs a few 
testcases...). Thanks.

> --------
>
> The library defines many things in the boost::detail and
> boost::detail::intimate namespaces that don't seem clearly
> function-related. What's our approach to guarantee that these names won't
> conflict with other parts of the boost library?
>
> 	boost::detail::intimate:
> 		SelectThen
> 		SelectElse
> 		Selector
> 	boost::detail:
> 		IF (possible conflict with non-boost macro here)
> 		is_void
> 		any_pointer
> 		unusable
> 		function_return_type
> 		functor_manager_operation_type
> 		clone_functor
> 		destroy_functor
> 		functor_manager
> 		count_if_used
> 		count_used_args
> 		undeletable
> 		has_empty_target

I should use a different namespace, perhaps boost::function::detail, for most 
of these. I'm hoping for IF and its helpers to disappear when the Boost 
metaprogramming library comes into existence...

> --------
>
> To make mixins compatible across versions of the function library, don't
> we need to specify the names that are reserved for use by members of the
> other classes that the mixin might conflict with?

Hmmm. That essentially stops the library from evolving, because we have to 
specify all of the internal names as well... I would expect that with a mixin 
the user would first cast to the type of the mixin before accessing members. 
The other way is more problematic: if the user overloads an internal 
function, we could see some strange behavior.

I'll look for more information from the mixin crowd to see what they do. 
Perhaps just a unique prefix on all internal members would suffice.

> --------
>
> The library defines a class nil_t and an object nil. While it's nice to
> have these around, they are not mentioned in the documentation, and I'm
> not sure they belong in the function library. And I think the library's
> design works fine without these -- it seems more a matter of personal
> taste that they are included.

They exist because we can't easily have:
boost::function<> f;
f = 0;

This requires a "const int" overload of operator=, which allows the 
nonsensical:
f = 7;

Even worse, we can't detect this abuse at compile-time, so it becomes a 
runtime issue.

nil/nil_t is meant to be freestanding and has almost nothing to do with 
boost::function. It is included as part of function because it is used there, 
but I think should be its own "library" (insofar as 6 lines constitutes a 
library...).

> The implementation provided for nil_t doesn't work for pointers-to-members,
>   so you're stuck using 0 instead of nil for those.

Good catch. Fixed it.

> Why is the object nil defined in an unnamed namespace nested inside the
> boost namespace? Doesn't this mean that we'll have separate (albeit small)
>   nil objects in each translation unit? Wouldn't it be better to share a
> single boost::nil object?

It would, but that requires the boost::nil object to be defined somewhere. In 
many cases, Boost libraries can be used without linking anything else, which 
is very, very convenient. I suspect that with the new build system we will 
want to have the option to do either. For instance, boost::nil would be 
declared through a macro that can do either
        1) declare it as extern. the definition of it will be compiled into the 
Boost library binary
        2) declare it in an anonymous namespace so each translation unit gets its 
own copy.

> --------
>
> The code that says "// Poison comparisons between functions" declares but
> does not define the == and != operators. Is this better than simply not
> defining the == and != operators for these particular parameter types? If
> this poisoning is necessary for == and !=, why isn't it necessary for
> other operators? Is there some way to make comparisons between functions
> fail at compile time rather than link time?

Good point... yes, I can make the operators private members of 
boost::function so using them would fail at compile time. 
The other operators aren't as much of a concern, because users accustomed to 
function pointers won't typically use operator< or operator+ on them. 
Function pointers support operator== and operator!=, so I wanted to 
explicitly poison them so it is certain that they are not (will not) be 
supported by boost::function.

> --------
>
> In BOOST_FUNCTION_FUNCTION::invoke, a const_cast is not needed to add a
> const qualifier to change a FunctionObj* to a const FunctionObj*.
> Assignment alone will do the trick.

Yep. Will do.

> --------
>
> The documentation has no substantive or motivating examples, so it's hard
> to figure out what kinds of things you'd use the library for.

Any place a function pointer or member function pointer would be used, 
function is a candidate. C++ programmers are very accustomed to using them, 
so I don't see that a substantive example is necessary. Examples of using 
mixins and policies would be worthwhile (see below).

> --------
>
> Many of the ifdefs at the top of function_base.hpp to define BOOST_
> symbols look like they belong in boost/config.hpp instead.

They do. They're in function_base.hpp so that I don't need to supply the 
whole config.hpp. I didn't want to tie the boost::function package to any 
specific version of the Boost libs while it was in development; the ifdefs 
will move to config.hpp at some later point in time.

> --------
>
> There is a specific ifdef for Metrowerks C++ in the implementation, but no
> mention of Metrowerks in the Portability section of the documentation. All
> the other compilers mentioned in the header are also mentioned in the
> documentation.

I don't actually have access to that compiler, so I'm afraid to claim that it 
works if I can't test it myself. I believe I got the thumbs-up from some of 
the Metrowerks users, so I could probably add it...

> --------
>
> I think Function should be accepted as part of Boost. But I do have some
> reservations.
>
> The advanced parts of the interface seem idiosyncratic. The pre-call and
> post-call provided by Policy seems just one special case of creating one
> kind of function object that calls through to another. Without a
> motivating example in the documentation or test programs, it's hard for me
> to see why this feature is included. The Mixin approach seems a clumsy way
> to add state, forcing a marriage of members between unrelated classes
> which is likely to lead to name conflicts, and requiring that the user
> also use the Policy mechanism to provide function code that has state.
> Where's the simple way to create a function with some state?

Mixins are a relatively common (and powerful) technique. It seems a 
reasonable way to add state, but I'm open to suggestions. Here's one nifty 
trick that mixins can do but other techniques to add state cannot:

struct MySlotCall {
  virtual int operator()(int, int) = 0;
  virtual int operator()(int, int) const = 0;
};

// later
boost::function<int, int, int>::mixin<MySlotCall>::type f;

When playing with a signals/slots prototype I found this to be quite useful 
indeed. I could claim that more classes should support mixins for just this 
reason.

I meant to add the following example to the documentation. I don't recall the 
syntax of the Boost.Threads library, but the motivation behind the 
precall/postcall policy was synchronization. Suppose we have the mixin and 
policy classes below:

struct synchronize_mixin {
  mutable boost::threads::recursive_mutex m;
};

struct synchronize_policy {
  void precall(const synchronize_mutex* m) { m->m.lock(); }
  void postcall(const synchronize_mutex* m) { m->m.unlock(); }
};

Using the mixin and policy together in a boost::function type synchronizes 
calls through the instances of that type. 

Other uses could include, for instance, detection of a dangerously high 
recursion depth (just stick a depth counter in and if it passes a certain 
threshold in precall through an exception). 

> The documentation has a lot of room for improvement, as I describe in
> detail above.
>
>      -- Darin

        Doug