[boost] Re: [review] hash functions

From: Daniel James (daniel_at_[hidden])
Date: 2005-03-09 07:16:44

Joaquín Mª López Muñoz wrote:
> Comments on the implementation.
> 
> 1. The overloads of hash_value for STL containers forces the implementation
> to include <vector>, <list>, etc., which makes the header a little
> heavier than desired. Of course, this could be avoided if only there
> exist <vector_fwd> style headers :( I propose that, instead of including
> the STL containers headers, they are forward declared:
> 
> namespace std{
> template<typenameT,typename A> class std::vector<T, A>;
> ....
> }
> 
> As is known, stdlib implementors are free to add extra template
> params, so this is not 100% safe, but:
> * All stdlibs that I'm aware of do not include extra template params
> * The forward declarations can be tweaked  for those stdlibs for
> which they don't match.

I'm not happy with this either.

Another possible solution is to just use Boost.Range on anything that 
'looks' like a container. But this will break on unordered containers 
(as equivalent containers can have different sequences).

> 2. It is said in hash.cpp that BCB has problems with the overload
> of hash_value for bool. This should be fixed in one way or another
> (possibly by not defining it for this compiler, I guess.)

It actually says that BCB has problems without it. There was an overload 
ambiguity error for boost::hash<bool> (although, calling 
boost::hash<bool> would be a very odd thing to do) and that was added to 
work around it.

> 3. Possible future enhancement: compatibility of hash_range
> with Boost.Range.

Yes, Thorsten asked for this as well. I think it's a good idea.

> Comments on the documentation.
> 
> 1. If this lib is going to claim adherence to Peter Dimov's proposal, the
> reference should state what the algortihms are for overloads of
> hash_value for:
> 
> int
> unsigned int
> long
> unsigned long
> std::pair
> std::basic_string
> STL containers
> 
> Currently all these overloads appear without a mention of what their
> exact behavior is. A mention to Peter's proposal is included, but I think
> it'd be better to replicate the spec entirely rather than rely on an external
> paper.

I didn't document these because I might change them in the future (I'll 
consult the list for opinions before I make any such change). But based 
on the feedback I've received so far, I probably should. Thorsten 
pointed out that it's important to guarentee that hash_range will give 
the same result as a string, so that it can be used with equivalent 
strings of different types. The same goes for sub-ranges of containers.

> 2. The overload of hash_value for bool is not documented. Its
> exact behavior should probably be documented, also.

That's because it's just there as workaround, I'll change it so that 
it's only defined for Borland.

> 3. The navigation buttons look cluttered (at least in my browser, IE 5.0.)

When added to boost it'll have the standard BoostBook look and feel.

> 4. Introduction: the "It is being proposed" thing should be removed if the
> lib is accepted.

Of course.

> 5. The docs talk of overloads for string and wstring, yet Peter's proposal
> and the implementation itself overload for basic_string<E,T,A>.

I'm following the resolution from issue 6.17 in the same paper.

> 6. hash_combine is incorrecly documented to return a std::size_t (it doesn't
> return anything.)

OK, I'll fix that. It would have been good to use Doxygen and avoid such 
documentation bugs, but it doesn't work very well with overloaded functions.

> Thanks to Daniel for his contribution! Best,

Thanks for your review, and for helping with the library.