[boost] Comments on functional/hash

From: Peter Dimov (pdimov_at_[hidden])
Date: 2005-04-02 06:03:52

First of all, nice work!

On headers:

* boost::hash defined in <boost/functional/hash.hpp>

Usually (when the number of top-level headers is small) we follow the rule 
that boost::X is defined in <boost/X.hpp>. This makes it much easier for 
users to remember what headers need to be included.

I think that hash.hpp needs to be a top-level header.

* hash/hash.hpp contains hash_range overloads for Range objects

I don't recall this being discussed during the review.

This introduces a dependency on the range headers. Given that std::pair is 
separated in its own header, this is a bit odd, especially when the range 
headers include <utility>.

But what's more important, these overloads are unnecessary. The proper 
protocol to obtain the hash value of an object x is hash_value( x ), not 
hash_range( x ). Consider the case std::vector<X>. If X does not support 
hash_value, hashing the vector will fail, even in the case where hash_range 
could have been used for X.

If a range object is not supported by hash_value, we just need to add a 
hash_value overload for it.

Summary: these overloads need to be removed.

* hash.hpp does not contain overloads for built-in arrays

This is a case that I missed when I prepared the hash_value proposal. We 
need to add them.

* boost::hash derived from std::unary_function

This introduces a completely unnecessary dependency on <functional>. 
boost::hash only needs to define result_type and argument_type.

* Header separation

Reviewers have expressed concerns that including all standard container 
headers would incur a significant hit. Is this the case in practice?

Currently, hash/hash.hpp results in 44247 lines, approx. 512K. The top-level 
hash.hpp results in 51096 lines, approx. 645K. In my opinion, this does not 
constitute an unacceptable increase.

In addition, defining all std-related overloads before hash_combine will 
eliminate the need for the call_hash workaround.

* The pointer overload for hash_value should probably be changed to take a 
void*

The reason for this is that on some platforms an int* and a void* to the 
same object may have different representations. On the other hand, this will 
disable the overload for function pointers. Maybe we need to leave the 
signature as-is but cast object pointers to void cv * internally. Or maybe 
the current implementation is good enough. Base* and Derived* to the same 
object may generally hash to different values anyway.

On tests:

* Tests do not test the specification

The tests are legitimate, but overly general. They test only the broad 
hash<>::operator() requirement, not a specific value. But our specification 
states precisely what hash value is to be expected (pointers and floats 
aside).

hash<X>( x ) needs to be tested against hash_value( x ), and hash_value( x ) 
needs to be tested against its expected value.

* custom_test

The portable definition of hash_value for custom objects is something along 
these lines:

namespace test
{

struct custom
{
#ifndef BOOST_NO_ARGUMENT_DEPENDENT_LOOKUP

    friend inline size_t hash_value { return ... }

#else

    size_t hash() const { return ... }

#endif
};

} // namespace test

#ifdef BOOST_NO_ARGUMENT_DEPENDENT_LOOKUP

namespace boost
{

size_t hash_value( test::custom const & x )
{
    return x.hash();
}

} // namespace boost

#endif

This is a bit more verbose but demonstrates the recommended approach for the 
two major cases, with or without argument dependent lookup.

Thanks for reading this far,

--
Peter Dimov
http://www.pdimov.com