From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B65CD3858D28 for ; Wed, 15 Dec 2021 21:21:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B65CD3858D28 Received: from mail-yb1-f200.google.com (mail-yb1-f200.google.com [209.85.219.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-246-3J6D8KvpM6ygCqCXurPMYA-1; Wed, 15 Dec 2021 16:21:23 -0500 X-MC-Unique: 3J6D8KvpM6ygCqCXurPMYA-1 Received: by mail-yb1-f200.google.com with SMTP id y17-20020a2586d1000000b005f6596e8760so45626203ybm.17 for ; Wed, 15 Dec 2021 13:21:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/4hRdZ2oSg2Ni+bAvhGHDGseRPH2DH5NzR6eVAN0fUA=; b=mwdXsaLY3H5DaQzfSGyk6iGVBfvZV2sDLRkoY6sXny4fGoCIEUNcgeExSy9QU2j7X7 dz6UEg44Ci+A7br/jeNtZkkSxfjd4E1QC/7ooWEPRYpgwcEfejL+GrXO2TES4CsubTzG 7ECE3t0YBVSPtKpQFnT+9+NrllaW5vW9jIk8dEnOqvjDiuIMpnyk8LLYpYdhjsokXuKC 0+nZtl9PS1QoGYs//ePdnG/YAmwhFa9acf9msLIxZdWh13x5+4sHYGI3eXyiPrXTCjKr rTq8D66+mbW0qeEtNZ/O7HZ/2G13EDyJ6FEtWorzns3Gxk5f5nGNyzEYJfwUSDxnz642 XQLw== X-Gm-Message-State: AOAM531Z6vkZ7BdWyPorzWFyS/hgRr4/r3GSazNi6lEXbJAkMi0UU+Em hO6urHb/FQgxaUpV2Cad3MU5shV1s7vVZY2kDTx3q/2jAu4OxZHOFdOkdNG4SdXF22j0WNME4Fk H1UFqRxOacWKxsVJst7V9GhVCLV+RPCo= X-Received: by 2002:a25:764c:: with SMTP id r73mr9164019ybc.107.1639603282523; Wed, 15 Dec 2021 13:21:22 -0800 (PST) X-Google-Smtp-Source: ABdhPJyF8MtHND9XTr8/KvD7Pmyf2hJre5lmpWscgU3fer0ltKmwc37V2/6RA7GR4Y4xNlN9mqv4cZ05mFHg6RBkesg= X-Received: by 2002:a25:764c:: with SMTP id r73mr9163995ybc.107.1639603282317; Wed, 15 Dec 2021 13:21:22 -0800 (PST) MIME-Version: 1.0 References: <57b54891-59ba-394d-3d53-218f5061b8ef@gmail.com> <9ea0770a-54fb-2c17-b527-2797b5fd3b50@gmail.com> <34b34b00-8c96-68c1-e436-ab0f64291c2f@gmail.com> <193032c8-8b9a-3e55-a244-075b0031d758@gmail.com> <5a951a74-5afd-a2da-02fd-9f9e7b33b448@gmail.com> <7a748ec5-b0b7-fb4d-312e-4a17b6bac8fa@gmail.com> In-Reply-To: <7a748ec5-b0b7-fb4d-312e-4a17b6bac8fa@gmail.com> From: Jonathan Wakely Date: Wed, 15 Dec 2021 21:21:10 +0000 Message-ID: Subject: Re: [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator> To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Jonathan Wakely , "libstdc++" , gcc-patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Dec 2021 21:21:28 -0000 On Wed, 15 Dec 2021 at 21:16, Fran=C3=A7ois Dumont w= rote: > Here is what I eventually would like to commit. > > I was not able to remove the _Safe_iterator_base branch in ptr_traits.h. > When adding the _Safe_iterator overload in C++20 and removing the branch > the 20_util/to_address/debug.cc test started to fail because it was not > calling my overload. I tried to declare the overload in ptr_traits.h > directly so it is known at the time it is used in std::to_address but the= n > it failed to match it with the implementation in safe_iterator.h. The > declaration was not easy to do and I guess I had it wrong. > > But it does not matter cause I think this version is the simplest one (as > it does not change a lot of code). > > libstdc++: Overload std::__to_address for __gnu_cxx::__normal_iterato= r. > > Prefer to overload __to_address to partially specialize > std::pointer_traits because > std::pointer_traits would be mostly useless. Moreover partial > specialization of > pointer_traits<__normal_iterator> fails to rebind C, so you get > incorrect types > like __normal_iterator>. In the case of > __gnu_debug::_Safe_iterator > the to_pointer method is impossible to implement correctly because we > are missing > the parent container to associate the iterator to. > > libstdc++-v3/ChangeLog: > > * include/bits/stl_iterator.h > (std::pointer_traits<__gnu_cxx::__normal_iterator<>>): Remove= . > (std::__to_address(const __gnu_cxx::__normal_iterator<>&)): > New for C++11 to C++17. > * include/debug/safe_iterator.h > (std::__to_address(const > __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>, _Sequence>&))= : > New for C++11 to C++17. > * testsuite/24_iterators/normal_iterator/to_address.cc: Add > check on std::vector::iterator > to validate both __gnu_cxx::__normal_iterator<> __to_address > overload in normal mode and > __gnu_debug::_Safe_iterator in _GLIBCXX_DEBUG mode. > > Tested under Linux x86_64 normal and _GLIBCXX_DEBUG modes for > C++11/C++14/C++17/C++20. > > Ok to commit ? > OK, thanks! > Fran=C3=A7ois > > > On 14/12/21 2:12 pm, Jonathan Wakely wrote: > > On Tue, 14 Dec 2021 at 06:53, Fran=C3=A7ois Dumont wrote: > >> Hi >> >> Any conclusion regarding this thread ? >> >> Fran=C3=A7ois >> >> >> On 06/10/21 7:25 pm, Fran=C3=A7ois Dumont wrote: >> > I forgot to ask if with this patch this overload: >> > >> > template >> > constexpr auto >> > __to_address(const _Ptr& __ptr, _None...) noexcept >> > { >> > if constexpr (is_base_of_v<__gnu_debug::_Safe_iterator_base, >> _Ptr>) >> > return std::__to_address(__ptr.base().operator->()); >> > else >> > return std::__to_address(__ptr.operator->()); >> > } >> > >> > should be removed ? >> >> > No, definitely not. > > That is the default overload for types that do not have a > pointer_traits::to_address specialization. If you remove it, __to_address > won't work for fancy pointers or any other pointer-like types. That would > completely break it. > > The purpose of C++20's std::to_address is to get a real pointer from a > pointer-like type. Using it with iterators is not the primary use case, b= ut > it needs to work with contiguous iterators because those are pointer-like= . > I made it work correctly with __normal_iterator because that was necessar= y > to support the uses of std::__to_address in and , but= I > removed those uses in: > > https://gcc.gnu.org/g:247bac507e63b32d4dc23ef1c55f300aafea24c6 > https://gcc.gnu.org/g:b83b810ac440f72e7551b6496539e60ac30c0d8a > > So now we don't really need the C++17 version of std::__to_address to wor= k > with __normal_iterator at all. > > I think it's OK to add the overload for __normal_iterator though, but onl= y > for C++11/14/17, because the default std::__to_address handles > __normal_iterator correctly in C++20. > > > > Or perhaps just the _Safe_iterator_base branch in it ? >> > > Yes, you can just remove that branch, because your new overload handles i= t. > > > > > >> > On 06/10/21 7:18 pm, Fran=C3=A7ois Dumont wrote: >> >> Here is another proposal with the __to_address overload. >> >> >> >> I preferred to let it open to any kind of __normal_iterator >> >> instantiation cause afaics std::vector supports fancy pointer types. >> >> It is better if __to_address works fine also in this case, no ? >> > > If we intend to support that, then we should verify it in the testsuite, > using __gnu_test::CustomPointerAlloc. > > > >> libstdc++: Overload std::__to_address for >> >> __gnu_cxx::__normal_iterator. >> >> >> >> Prefer to overload __to_address to partially specialize >> >> std::pointer_traits because >> >> std::pointer_traits would be mostly useless. In the case of >> >> __gnu_debug::_Safe_iterator >> >> the to_pointer method is even impossible to implement correctly >> >> because we are missing >> >> the parent container to associate the iterator to. >> > > To record additional rationale in the git history, please add that the > partial specialization of pointer_traits<__normal_iterator> fails t= o > rebind C, so you get incorrect types like __normal_iterator vector>. > > > >> >> >> libstdc++-v3/ChangeLog: >> >> >> >> * include/bits/stl_iterator.h >> >> (std::pointer_traits<__gnu_cxx::__normal_iterator<>>): Remove. >> > > OK to remove this (it's broken anyway). > > >> (std::__to_address(const >> >> __gnu_cxx::__normal_iterator<>&)): New. >> > > Please make this only defined for C++11, 14 and 17. > > >> >> * include/debug/safe_iterator.h >> >> (std::__to_address(const >> >> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>, >> >> _Sequence>&)): >> >> New. >> > > OK to add this (including for C++20), and remove the _Safe_iterator branc= h > from the C++20 std::__to_address in . > > I think this new overload could return > std::__to_address(__it.base().base()) though. That saves a function call, > by going directly to the value stored in the __normal_iterator. > > > >> >> * testsuite/24_iterators/normal_iterator/to_address.cc: >> >> Add check on std::vector::iterator >> >> to validate both __gnu_cxx::__normal_iterator<> >> >> __to_address overload in normal mode and the >> > > Add similar checks for vector>. > > OK with those changes, thanks. > > > > >