[libcxx] r296919 - Fix hash requirements check in __hash_table.
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 3 15:56:41 PST 2017
On Fri, Mar 3, 2017 at 4:01 PM, Nico Weber <thakis at chromium.org> wrote:
> "the specified hash does not meet the Hash requirements" isn't a very
> actionable diagnostic.
>
True, For now I was only trying to out-diagnose template barf when I wrote
it initially. So in that regard it's a bit more actionable that it was, at
least I hope.
I'm planning to create some machinery to check and diagnose every hash
requirement separately, but I was going to do that separately. For instance
some requirements could be diagnosed as warnings instead of hard errors,
allowing users to keep their code compiling until they have time to fix it.
Of course patches are always welcome :-P
/Eric
> Is it possible to use some warning text that lets people know what they
> need to do to make the compiler happy, instead of just telling them that
> the compiler is unhappy?
>
> On Fri, Mar 3, 2017 at 5:35 PM, Eric Fiselier via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: ericwf
>> Date: Fri Mar 3 16:35:58 2017
>> New Revision: 296919
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=296919&view=rev
>> Log:
>> Fix hash requirements check in __hash_table.
>>
>> r296565 attempted to add better diagnostics when an unordered container
>> is instantiated with a hash that doesn't meet the Hash requirements.
>>
>> However I mistakenly checked the wrong set of requirements. Specifically
>> it checked if the hash met the requirements for specializations of
>> std::hash. However these requirements are stricter than the generic
>> Hash requirements.
>>
>> This patch fixes the assertions to only check the Hash requirements.
>>
>> Modified:
>> libcxx/trunk/include/__hash_table
>> libcxx/trunk/include/utility
>> libcxx/trunk/test/libcxx/containers/unord/unord.set/missing_
>> hash_specialization.fail.cpp
>>
>> Modified: libcxx/trunk/include/__hash_table
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__
>> hash_table?rev=296919&r1=296918&r2=296919&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/include/__hash_table (original)
>> +++ libcxx/trunk/include/__hash_table Fri Mar 3 16:35:58 2017
>> @@ -871,16 +871,15 @@ public:
>> template <class _Key, class _Hash, class _Equal, class _Alloc>
>> struct __diagnose_hash_table_helper {
>> static constexpr bool __trigger_diagnostics()
>> - _LIBCPP_DIAGNOSE_WARNING(__has_enabled_hash<_Key, _Hash>::value
>> + _LIBCPP_DIAGNOSE_WARNING(__check_hash_requirements<_Key,
>> _Hash>::value
>> && !__invokable<_Hash const&, _Key
>> const&>::value,
>> "the specified hash functor does not provide a const call
>> operator")
>> _LIBCPP_DIAGNOSE_WARNING(is_copy_constructible<_Equal>::value
>> && !__invokable<_Equal const&, _Key const&,
>> _Key const&>::value,
>> "the specified comparator type does not provide a const call
>> operator")
>> {
>> - static_assert(__has_enabled_hash<_Key, _Hash>::value,
>> - "the specified hash functor does not meet the requirements for an "
>> - "enabled hash");
>> + static_assert(__check_hash_requirements<_Key, _Hash>::value,
>> + "the specified hash does not meet the Hash requirements");
>> static_assert(is_copy_constructible<_Equal>::value,
>> "the specified comparator is required to be copy constructible");
>> return true;
>>
>> Modified: libcxx/trunk/include/utility
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/uti
>> lity?rev=296919&r1=296918&r2=296919&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/include/utility (original)
>> +++ libcxx/trunk/include/utility Fri Mar 3 16:35:58 2017
>> @@ -1560,14 +1560,19 @@ struct _LIBCPP_TEMPLATE_VIS hash<nullptr
>> #endif
>>
>> #ifndef _LIBCPP_CXX03_LANG
>> -template <class _Key, class _Hash = std::hash<_Key> >
>> -using __has_enabled_hash = integral_constant<bool,
>> - is_default_constructible<_Hash>::value &&
>> +template <class _Key, class _Hash>
>> +using __check_hash_requirements = integral_constant<bool,
>> is_copy_constructible<_Hash>::value &&
>> is_move_constructible<_Hash>::value &&
>> __invokable_r<size_t, _Hash, _Key const&>::value
>> >;
>>
>> +template <class _Key, class _Hash = std::hash<_Key> >
>> +using __has_enabled_hash = integral_constant<bool,
>> + __check_hash_requirements<_Key, _Hash>::value &&
>> + is_default_constructible<_Hash>::value
>> +>;
>> +
>> #if _LIBCPP_STD_VER > 14
>> template <class _Type, class>
>> using __enable_hash_helper_imp = _Type;
>>
>> Modified: libcxx/trunk/test/libcxx/containers/unord/unord.set/missing_
>> hash_specialization.fail.cpp
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/libcxx
>> /containers/unord/unord.set/missing_hash_specialization.
>> fail.cpp?rev=296919&r1=296918&r2=296919&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/test/libcxx/containers/unord/unord.set/missing_hash_specialization.fail.cpp
>> (original)
>> +++ libcxx/trunk/test/libcxx/containers/unord/unord.set/missing_hash_specialization.fail.cpp
>> Fri Mar 3 16:35:58 2017
>> @@ -23,14 +23,48 @@
>> #include <utility>
>>
>> using VT = std::pair<int, int>;
>> -using Set = std::unordered_set<VT>;
>> +
>> +struct BadHashNoCopy {
>> + BadHashNoCopy() = default;
>> + BadHashNoCopy(BadHashNoCopy const&) = delete;
>> +
>> + template <class T>
>> + size_t operator()(T const&) const { return 0; }
>> +};
>> +
>> +struct BadHashNoCall {
>> +
>> +};
>> +
>> +
>> +struct GoodHashNoDefault {
>> + explicit GoodHashNoDefault(void*) {}
>> + template <class T>
>> + size_t operator()(T const&) const { return 0; }
>> +};
>>
>> int main() {
>>
>> - Set s; // expected-error at __hash_table:* {{the specified hash functor
>> does not meet the requirements for an enabled hash}}
>> + {
>> + using Set = std::unordered_set<VT>;
>> + Set s; // expected-error at __hash_table:* {{the specified hash does
>> not meet the Hash requirements}}
>> +
>>
>> // FIXME: It would be great to suppress the below diagnostic all
>> together.
>> // but for now it's sufficient that it appears last. However
>> there is
>> // currently no way to test the order diagnostics are issued.
>> // expected-error at memory:* {{call to implicitly-deleted default
>> constructor of 'std::__1::hash<std::__1::pair<int, int> >'}}
>> + }
>> + {
>> + using Set = std::unordered_set<int, BadHashNoCopy>;
>> + Set s; // expected-error at __hash_table:* {{the specified hash does
>> not meet the Hash requirements}}
>> + }
>> + {
>> + using Set = std::unordered_set<int, BadHashNoCall>;
>> + Set s; // expected-error at __hash_table:* {{the specified hash does
>> not meet the Hash requirements}}
>> + }
>> + {
>> + using Set = std::unordered_set<int, GoodHashNoDefault>;
>> + Set s(/*bucketcount*/42, GoodHashNoDefault(nullptr));
>> + }
>> }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170303/e97a45e3/attachment.html>
More information about the cfe-commits
mailing list