"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