From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32018 invoked by alias); 7 May 2019 12:22:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 32010 invoked by uid 89); 7 May 2019 12:22:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=__cxx11 X-HELO: mail-ua1-f65.google.com Received: from mail-ua1-f65.google.com (HELO mail-ua1-f65.google.com) (209.85.222.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 May 2019 12:21:59 +0000 Received: by mail-ua1-f65.google.com with SMTP id s30so5917417uas.8 for ; Tue, 07 May 2019 05:21:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=2rXpY+fxx7+XNAGCg7g6HtIGMFOW3FEIblDPKwZYjoI=; b=bdMValhGrNaW6iFkYWBusuJHAoXKIsV53KaTkoLQeJNEczHS4Wy/R8UplvURmlMYiB 4yWafrZRKctoBiDCTlanNLHtb4EZ6POjLE6AdpcjrXZEGICLmlEi1tz9XtcfZZjt8xP7 kkvFJuRLjDGdiXlDjzzLcXmJQRtj7Dao6KvgMXUUALOVanNWWeiKRCLkWmNpSEf9qj8Z KjBUBik1OIkQKxjKbxam3qDTJF5awlJ18kq+Qk84YkF7ZqzQr78tdF9Jn6wlHgRSAo3F IqApe1DclO6TdXU4ABOHrDbq1WvCcpYuxhrwTtP/lU+pJ+OTvuBL1WGcvyYeT41WjbNv h6ng== MIME-Version: 1.0 References: <20170323174905.GG4425@redhat.com> <20190503224255.GI2599@redhat.com> <20190504143628.GL2599@redhat.com> <20190507093746.GQ2599@redhat.com> <20190507100740.GT2599@redhat.com> In-Reply-To: <20190507100740.GT2599@redhat.com> From: Christophe Lyon Date: Tue, 07 May 2019 12:22:00 -0000 Message-ID: Subject: Re: [PATCH] Implement LWG 2686, hash To: Jonathan Wakely Cc: =?UTF-8?Q?Daniel_Kr=C3=BCgler?= , "libstdc++" , gcc Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00292.txt.bz2 On Tue, 7 May 2019 at 12:07, Jonathan Wakely 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 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=C3=BCgler wrote: > >>>>>>The following is an *untested* patch suggestion, please verify. > >>>>>> > >>>>>>Notes: My interpretation is that hash 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 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. > >>>>> > >>>>>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 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 > >>>>>Date: Thu Mar 23 11:47:39 2017 +0000 > >>>>> > >>>>> Implement LWG 2686, std::hash, for C++17 > >>>>> 2017-03-23 Daniel Kruegler > >>>>> Implement LWG 2686, Why is std::hash specialized for error_cod= e, > >>>>> but not error_condition? > >>>>> * include/std/system_error (hash): Define for= C++17. > >>>>> * testsuite/20_util/hash/operators/size_t.cc (hash): > >>>>> Instantiate test for error_condition. > >>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc > >>>>> (hash): Instantiate hash. > >>>>> > >>>>>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/in= clude/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 > >>>>> > >>>>>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 > >>>>>+ : public __hash_base > >>>>>+ { > >>>>>+ size_t > >>>>>+ operator()(const error_condition& __e) const noexcept > >>>>>+ { > >>>>>+ const size_t __tmp =3D 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 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/a= rm-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/a= rm-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/a= rm-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/a= rm-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/a= rm-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'