[boost] Re: [review] hash functions

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2005-03-09 06:33:43

"Thorsten Ottosen" wrote:

> It's a pleasure to announce that the review of Daniel James'
> hash functions library...
>
Just few comments and questions from quick playing.

/Pavel

______________________________________________
1. Woudn't it be better to have separate
   headers as hash_for_sets.hpp, hash_for_maps.hpp etc
   instead of one header including the whole world?

   This way other Boost containers could be added
   w/o affecting compile time.

   I remember long debates about this during
   Serialization review. These resulted in solution
   with separate headers + common header for all.

   And if possible, other containers (slist,
   few Boost ones) should be added.

   It may be considered whether support for
   shared_ptr etc could be added.

______________________________________________
2. The reference documentation says string
   and wstring are passed by value, while they
   are by const&.

______________________________________________
3. __int64/long long should be supported.

______________________________________________
4. What if I would like hash result other
   than size_t, frex unsigned char?

   I may be memory constrained or may want to save
   data on disk.

______________________________________________
5. namespace detail would be better hash_detail
   to avoid possible clashes.

______________________________________________
6. Maybe the struct hash::operator() could use
   boost::call_traits<T>::param_type to
   optimize value passing for primitive types.

______________________________________________
7. compiling hash_float_test.cpp with Intel C++ 7.0
   plugged in VC6 IDE (with old Dinkumware) I get:

hash_float_test.cpp
C:\work\reviews\hash\hash\libs\functional\hash\test\hash_float_test.cpp(56):
error: identifier "denorm_present" is undefined
          if(std::numeric_limits<T>::has_denorm == denorm_present) {

BCB 6.4 with STLport 4.5.1 says:

[Linker Error] Unresolved external '_STL::_LimG<bool>::_F_inf' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ
[Linker Error] Unresolved external '_STL::_LimG<bool>::_F_qNaN' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ
[Linker Error] Unresolved external '_STL::_LimG<bool>::_D_inf' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ
[Linker Error] Unresolved external '_STL::_LimG<bool>::_D_qNaN' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ
[Linker Error] Unresolved external '_STL::_LimG<bool>::_L_inf' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ
[Linker Error] Unresolved external '_STL::_LimG<bool>::_L_qNaN' referenced 
from C:\WORK\REVIEWS\HASH\HASH\LIBS\FUNCTIONAL\HASH\TEST\HASH_FLOAT_TEST.OBJ

______________________________________________
8. old Dinkumware fix: hash_custom_test.cpp:

return hasher(std::tolower(x));

==>>

using namespace std;
return hasher(tolower(x));

______________________________________________
9. Win32 Intel fix: by default I get error:

C:\work\reviews\hash\hash\boost/functional/hash.hpp(226): error:
no instance of overloaded function "boost::hash_value" matches the argument 
list
            argument types are: (const test::custom)
              return hash_value(val);

one needs to add compiler option:
    /Qoption,c,--arg_dep_lookup

to enable ADL with Intel when emulating VC6.

______________________________________________
10. performance: wouldn't it be better to provide
    hand written specialization for std::string/wstring
    instead of relying on hash_range/combine?

    The compiler may not optimize chain of functions
    out/pass parameters in most efficient way/
    keep most data in registers.

    It's just feeling, I didn't do any measurements.

______________________________________________
11. VC6 warning: just for completeness

hash_number_test.cpp
c:\boost\boost_1_32_0\boost\test\test_tools.hpp(428) :
warning C4805: '==' : unsafe mix of type 'const int' and type 'const bool' 
in operation
        c:\work\reviews\hash\hash\libs\functional\hash\test\hash_number_test.cpp(48) 
:
see reference to function template instantiation 'bool __cdecl 
boost::test_tools::tt_detail::equal_and_continue_impl(const int &,const bool 
&,class boost::basic_w
rap_stringstream<char> &,class boost::unit_test::basic_cstring<char const 
 >,unsigned int,enum boost::unit_test::log_level,unsigned int)' being 
compiled

______________________________________________
12. BCB compiles and runs all tests (w/o floats).
    Intel 7 dtto (with the fix from [9] for custom test).
    VC6 doesn't floats and custom test (due to lack ADL).

______________________________________________
13. The headers may have

#if (defined _MSC_VER) && (_MSC_VER >= 1200)
#  pragma once
#endif

on the top.

______________________________________________
14. some rationale may be given for hash function

  seed ^= hash_value(v) + (seed<<6) + (seed>>2);

- link where does it come from or so.

If one creates different algorithm the code
may be prepared for such situation by:

#ifdef BOOST_HASH_USER_HASH_COMBINE_OFFERED
   BOOST_HASH_USER_HASH_COMBINE_OFFERED;
#else
  seed ^= hash_value(v) + (seed<<6) + (seed>>2);
#endif

or something like that.

______________________________________________
15. Complete example how to write e.g. case
    insensitive hash and how to plug it in should
    be shown in docs.

______________________________________________
EOF