[PATCH] Implement LWG 2686, hash<error_condition>
Szabolcs Nagy
Szabolcs.Nagy@arm.com
Thu May 9 14:43:00 GMT 2019
On 07/05/2019 13:21, Christophe Lyon wrote:
> On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> On 07/05/19 10:37 +0100, Jonathan Wakely wrote:
>>> On 07/05/19 11:05 +0200, Christophe Lyon wrote:
>>>> On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>
>>>>> On 03/05/19 23:42 +0100, Jonathan Wakely wrote:
>>>>>> On 23/03/17 17:49 +0000, Jonathan Wakely wrote:
>>>>>>> On 12/03/17 13:16 +0100, Daniel Krügler wrote:
>>>>>>>> The following is an *untested* patch suggestion, please verify.
>>>>>>>>
>>>>>>>> Notes: My interpretation is that hash<error_condition> should be
>>>>>>>> defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please
>>>>>>>> double-check that course of action.
>>>>>>>
>>>>>>> That's right.
>>>>>>>
>>>>>>>> I noticed that the preexisting hash<error_code> did directly refer to
>>>>>>>> the private members of error_code albeit those have public access
>>>>>>>> functions. For consistency I mimicked that existing style when
>>>>>>>> implementing hash<error_condition>.
>>>>>>>
>>>>>>> I see no reason for that, so I've removed the friend declaration and
>>>>>>> used the public member functions.
>>>>>>
>>>>>> I'm going to do the same for hash<error_code> too. It can also use the
>>>>>> public members instead of being a friend.
>>>>>>
>>>>>>
>>>>>>> Although this is a DR, I'm treating it as a new C++17 feature, so I've
>>>>>>> adjusted the patch to only add the new specialization for C++17 mode.
>>>>>>> We're too close to the GCC 7 release to be adding new things to the
>>>>>>> default mode, even minor things like this. After GCC 7 is released we
>>>>>>> can revisit it and decide if we want to enable it for all modes.
>>>>>>
>>>>>> We never revisited that, and it's still only enabled for C++17 and up.
>>>>>> I guess that's OK, but we could enabled it for C++11 and 14 on trunk
>>>>>> if we want. Anybody care enough to argue for that?
>>>>>>
>>>>>>> Here's what I've tested and will be committing.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>> commit 90ca0fd91f5c65af370beb20af06bdca257aaf63
>>>>>>> Author: Jonathan Wakely <jwakely@redhat.com>
>>>>>>> Date: Thu Mar 23 11:47:39 2017 +0000
>>>>>>>
>>>>>>> Implement LWG 2686, std::hash<error_condition>, for C++17
>>>>>>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com>
>>>>>>> Implement LWG 2686, Why is std::hash specialized for error_code,
>>>>>>> but not error_condition?
>>>>>>> * include/std/system_error (hash<error_condition>): Define for C++17.
>>>>>>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>):
>>>>>>> Instantiate test for error_condition.
>>>>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc
>>>>>>> (hash<error_condition>): Instantiate hash<error_condition>.
>>>>>>>
>>>>>>> diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error
>>>>>>> index 6775a6e..ec7d25f 100644
>>>>>>> --- a/libstdc++-v3/include/std/system_error
>>>>>>> +++ b/libstdc++-v3/include/std/system_error
>>>>>>> @@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>>>> _GLIBCXX_END_NAMESPACE_VERSION
>>>>>>> } // namespace
>>>>>>>
>>>>>>> -#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
>>>>>>> -
>>>>>>> #include <bits/functional_hash.h>
>>>>>>>
>>>>>>> namespace std _GLIBCXX_VISIBILITY(default)
>>>>>>> {
>>>>>>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>>>>
>>>>>>> +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
>>>>>>> // DR 1182.
>>>>>>> /// std::hash specialization for error_code.
>>>>>>> template<>
>>>>>>> @@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>>>> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp);
>>>>>>> }
>>>>>>> };
>>>>>>> +#endif // _GLIBCXX_COMPATIBILITY_CXX0X
>>>>>>> +
>>>>>>> +#if __cplusplus > 201402L
>>>>>>> + // DR 2686.
>>>>>>> + /// std::hash specialization for error_condition.
>>>>>>> + template<>
>>>>>>> + struct hash<error_condition>
>>>>>>> + : public __hash_base<size_t, error_condition>
>>>>>>> + {
>>>>>>> + size_t
>>>>>>> + operator()(const error_condition& __e) const noexcept
>>>>>>> + {
>>>>>>> + const size_t __tmp = std::_Hash_impl::hash(__e.value());
>>>>>>> + return std::_Hash_impl::__hash_combine(__e.category(), __tmp);
>>>>>>
>>>>>> When I changed this from using __e._M_cat (as in Daniel's patch) to
>>>>>> __e.category() I introduced a bug, because the former is a pointer to
>>>>>> the error_category (and error_category objects are unique and so can
>>>>>> be identified by their address) and the latter is the object itself,
>>>>>> so we hash the bytes of an abstract base class instead of hashing the
>>>>>> pointer to it. Oops.
>>>>>>
>>>>>> Patch coming up to fix that.
>>>>>
>>>>> Here's the fix. Tested powerpc64le-linux, committed to trunk.
>>>>>
>>>>> I'll backport this to 7, 8 and 9 as well.
>>>>>
>>>>
>>>> Hi Jonathan,
>>>>
>>>> Does the new test lack dg-require-filesystem-ts ?
>>>
>>> It lacks it, because it doesn't use the filesystem library at all.
>>>
>>>> I'm seeing link failures on arm-eabi (using newlib):
>>>> Excess errors:
>>>> /libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir'
>>>> /libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir'
>>>> /libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod'
>>>> /libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined
>>>> reference to `chmod'
>>>> /libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf'
>>>> /libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd'
>>>>
>>>> Christophe
>>
>> Is it definitely the new 19_diagnostics/error_condition/hash.cc test
>> that's giving this error?
i looked at this and ld -M reports
/B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o)
hash.o (std::_V2::error_category::default_error_condition(int) const)
/B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o)
/B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) (void std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag))
...
i.e. hash.o pulls system_error.o in because of
std::_V2::error_category::default_error_condition(int) const
and system_error.o pulls fs_ops.o in because of
std::__cxx11::basic_string...
symbol reference.
it seems fs_ops.o is the first object in libstdc++.a
that provides a (weak) definition for
_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE12_M_constructIPKcEEvT_S8_St20forward_iterator_tag
>>
>> I adjusted the pre-existing 27_io/filesystem/operations/absolute.cc
>> test in r270874, which seems a more likely culprit, but that already
>> has dg-require-filesystem-ts.
>>
>>
>
> Yes, and there full errors are:
> /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o):
> In function `std::filesystem::current_path(std::filesystem::__cxx11::path
> const&, std::error_code&)':
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:806:
> undefined reference to `chdir'
> /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o):
> In function `(anonymous
> namespace)::create_dir(std::filesystem::__cxx11::path const&,
> std::filesystem::perms, std::error_code&)':
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:583:
> undefined reference to `mkdir'
> /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o):
> In function `std::filesystem::permissions(std::filesystem::__cxx11::path
> const&, std::filesystem::perms, std::filesystem::perm_options,
> std::error_code&)':
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:1134:
> undefined reference to `chmod'
> /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o):
> In function `std::filesystem::do_copy_file(char const*, char const*,
> std::filesystem::copy_options_existing_file, stat*, stat*,
> std::error_code&)':
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439:
> undefined reference to `chmod'
> /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o):
> In function `std::filesystem::current_path[abi:cxx11](std::error_code&)':
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:750:
> undefined reference to `pathconf'
> /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:769:
> undefined reference to `getcwd'
>
More information about the Gcc-patches
mailing list