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.