Boost logo

Boost :

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

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

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

    friend inline size_t hash_value { return ... }


    size_t hash() const { return ... }


} // namespace test


namespace boost

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

} // namespace boost


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 

Boost list run by bdawes at, gregod at, cpdaniel at, john at