Created attachment 26090 [details] Declare std::hash<T> instead of defining it. libstdc++'s current definition of the unspecialized std::hash template gives bad error messages when a user forgets to define a hash function for their type. Specifically, providing a declaration but no definition of operator() moves the error from compile to link time: $ cat test.cc #include <unordered_set> struct MyStruct { int i; }; bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.i == rhs.i; } int main() { std::unordered_set<MyStruct> s; s.insert(MyStruct{3}); } $ g++ -g -std=c++11 test.cc -o test /tmp/cclzhwaU.o: In function `std::__detail::_Hash_code_base<MyStruct, MyStruct, std::_Identity<MyStruct>, std::equal_to<MyStruct>, std::hash<MyStruct>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, true>::_M_hash_code(MyStruct const&) const': .../include/c++/4.7.0/bits/hashtable_policy.h:702: undefined reference to `std::hash<MyStruct>::operator()(MyStruct) const' collect2: error: ld returned 1 exit status With the attached patch, the error is much more informative, if not exactly concise: $ g++ -g -std=c++11 test.cc -o test In file included from .../include/c++/4.7.0/bits/hashtable.h:35:0, from .../include/c++/4.7.0/unordered_set:45, from test.cc:1: .../include/c++/4.7.0/bits/hashtable_policy.h: In instantiation of ‘struct std::__detail::_Hash_code_base<MyStruct, MyStruct, std::_Identity<MyStruct>, std::equal_to<MyStruct>, std::hash<MyStruct>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, true>’: .../include/c++/4.7.0/bits/hashtable.h:149:11: required from ‘class std::_Hashtable<MyStruct, MyStruct, std::allocator<MyStruct>, std::_Identity<MyStruct>, std::equal_to<MyStruct>, std::hash<MyStruct>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, true, true, true>’ .../include/c++/4.7.0/bits/unordered_set.h:46:11: required from ‘class std::__unordered_set<MyStruct, std::hash<MyStruct>, std::equal_to<MyStruct>, std::allocator<MyStruct>, true>’ .../include/c++/4.7.0/bits/unordered_set.h:277:11: required from ‘class std::unordered_set<MyStruct>’ test.cc:9:32: required from here .../include/c++/4.7.0/bits/hashtable_policy.h:740:20: error: ‘std::__detail::_Hash_code_base<_Key, _Value, _ExtractKey, _Equal, _H1, _H2, std::__detail::_Default_ranged_hash, true>::_M_h1’ has incomplete type ... In particular, the "required from here" line points at the actual source location that needs to be able to find the definition. I believe the patch is allowed by C++11 since I can't find a specification of the contents of the unspecialized template, and libc++ uses basically the same technique: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__functional_base?view=markup. Another way to accomplish something similar would be to delete the operator() declaration from the std::hash<T> definition. I believe that's not as good because it produces error messages saying that std::hash lacks an operator() rather than that the template has incomplete type. Better yet would be finding a way to include "please specialize me" in the error message.
Could we put a static_assert in the default operator() definition?
For a while, back in tr1 times, we definitely had just a declaration for the primary hash. Then at some point things changed, I can't remember in which circumstance. If nothing breaks I think we can certainly go back to it. I'll regtest the patchlet.
Evidently the patchlet has not been tested ;) The library doesn't even build because we have code in src/ specializing the operator which would have to be adjusted. All in all, I guess what Jon suggested should not lead to worse error messages, and seems very easily doable and safe. Jon, would you like to suggest the text of the static_assert?
Would the condition of the static_assert need to depend on the template parameter somehow, to avoid the case where no valid specialization can ever be generated? (Which would be ill-formed, no diagnostic required, by [temp.res] p8.) e.g. static_assert( integral_constant<int, sizeof(_Tp)>::value, "std::hash must be specialized for this type" )
Uhmm, I don't know, probably yes. If we are in doubt we can as well use what I'm going to attach, which resolves the src/ fallback. Which change leads to the best error messages?
Created attachment 26098 [details] Completed patch
The static_assert definitely improves the error message. I'm going to prepare a combined patch (testcases in 23_containers/unordered_* have also to be tweaked)
Author: paolo Date: Thu Dec 15 22:15:21 2011 New Revision: 182392 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182392 Log: 2011-12-15 Paolo Carlini <paolo.carlini@oracle.com> Jonathan Wakely <jwakely.gcc@gmail.com> PR libstdc++/51558 * include/bits/functional_hash.h (struct hash): Add static_assert. * src/compatibility-c++0x.cc: Adjust compatibility definitions. * testsuite/23_containers/unordered_map/erase/51142.cc: Adjust. * testsuite/23_containers/unordered_set/erase/51142.cc: Likewise. * testsuite/23_containers/unordered_multimap/erase/51142.cc: Likewise. * testsuite/23_containers/unordered_multiset/erase/51142.cc: Likewise. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/functional_hash.h trunk/libstdc++-v3/src/compatibility-c++0x.cc trunk/libstdc++-v3/testsuite/23_containers/unordered_map/erase/51142.cc trunk/libstdc++-v3/testsuite/23_containers/unordered_multimap/erase/51142.cc trunk/libstdc++-v3/testsuite/23_containers/unordered_multiset/erase/51142.cc trunk/libstdc++-v3/testsuite/23_containers/unordered_set/erase/51142.cc
Done.