From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id 5FFFC3858D28; Wed, 15 Dec 2021 21:16:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5FFFC3858D28 Received: by mail-wr1-x430.google.com with SMTP id a9so40424558wrr.8; Wed, 15 Dec 2021 13:16:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=4YgyJa7TbKWcx7RqOvU9NfyOfBtM25K0dj7SnD986L0=; b=Yqyoyq33e9I4eLnOT44t7aIGq8+SP/Py3QzUPJEdAJD4rhISm21NX3EtXRpWEREara jqCnGiIj6LbH05ulrNotInyI+AidPEqw0iiHF7N9KUe40b8XRLpVFANhUQxLtYPh/FUT WB5DvOhbx3eC5c2nIi9REmKbcy8ootBAuDH9saHNhundJD9uOBa7T/ijYiC8UUD5UqIy O9WQFWYB0y2VSMuWKh237VCVeIK41WJ0dzLQsPo3ur1GbKAR4qjnEZcV5mVcvoGAdnwT UTIXryCdJoSAt1yOpNz7ISwmzXZEaWFiPKibQA64f4WDEk5QPvyzlJxVNwNfkvC/avO4 FBng== X-Gm-Message-State: AOAM530FUDdY7XkAONKXDskYHr0n8tE7hiAMrappDCayDKNm18p76xFh GHHt10u0sPpQtdZHsjQCEhchMw2wKiY= X-Google-Smtp-Source: ABdhPJwCgyjBtcZBxgnH3kjZ64xtHLkxFyfypCkiXDAxEPXg474leBrtxdixzl+XhI+g3YHLPJQXtQ== X-Received: by 2002:adf:fbc1:: with SMTP id d1mr6406804wrs.648.1639603005013; Wed, 15 Dec 2021 13:16:45 -0800 (PST) Received: from [10.40.5.105] ([109.190.253.11]) by smtp.googlemail.com with ESMTPSA id l3sm2848283wmq.46.2021.12.15.13.16.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Dec 2021 13:16:44 -0800 (PST) Subject: Re: [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator> To: Jonathan Wakely Cc: Jonathan Wakely , libstdc++ , gcc-patches 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> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <7a748ec5-b0b7-fb4d-312e-4a17b6bac8fa@gmail.com> Date: Wed, 15 Dec 2021 22:16:42 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------ECF552303D1F419634C5DF99" Content-Language: fr X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, KAM_LOTSOFHASH, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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 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:16:49 -0000 This is a multi-part message in MIME format. --------------ECF552303D1F419634C5DF99 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 then 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_iterator.     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 ? François On 14/12/21 2:12 pm, Jonathan Wakely wrote: > On Tue, 14 Dec 2021 at 06:53, François Dumont wrote: > > Hi > >      Any conclusion regarding this thread ? > > François > > > On 06/10/21 7:25 pm, François 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, but it needs to work with contiguous iterators because those are > pointer-like. I made it work correctly with __normal_iterator because > that was necessary 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 > work with __normal_iterator at all. > > I think it's OK to add the overload for __normal_iterator though, but > only 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 it. > > > > > > > On 06/10/21 7:18 pm, François 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 to rebind C, so you get incorrect types like > __normal_iterator>. > > > >> > >>     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 > branch 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. --------------ECF552303D1F419634C5DF99 Content-Type: text/x-patch; charset=UTF-8; name="to_address.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="to_address.patch" diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index 6bd860b803e..ac9342112f4 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1349,32 +1349,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #if __cplusplus >= 201103L - // Need to specialize pointer_traits because the primary template will - // deduce element_type of __normal_iterator as T* rather than T. +#if __cplusplus <= 201703L + // Need to overload __to_address because the pointer_traits primary template + // will deduce element_type of __normal_iterator as T* rather than T. template - struct pointer_traits<__gnu_cxx::__normal_iterator<_Iterator, _Container>> - { - private: - using _Base = pointer_traits<_Iterator>; - - public: - using element_type = typename _Base::element_type; - using pointer = __gnu_cxx::__normal_iterator<_Iterator, _Container>; - using difference_type = typename _Base::difference_type; - - template - using rebind = __gnu_cxx::__normal_iterator<_Tp, _Container>; - - static pointer - pointer_to(element_type& __e) noexcept - { return pointer(_Base::pointer_to(__e)); } - -#if __cplusplus >= 202002L - static element_type* - to_address(pointer __p) noexcept - { return __p.base(); } + constexpr auto + __to_address(const __gnu_cxx::__normal_iterator<_Iterator, + _Container>& __it) noexcept + -> decltype(std::__to_address(__it.base())) + { return std::__to_address(__it.base()); } #endif - }; /** * @addtogroup iterators diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index 5584d06de5a..9c821c82e17 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -1013,6 +1013,23 @@ namespace __gnu_debug } // namespace __gnu_debug +#if __cplusplus >= 201103L && __cplusplus <= 201703L +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + + template + constexpr auto + __to_address(const __gnu_debug::_Safe_iterator< + __gnu_cxx::__normal_iterator<_Iterator, _Container>, + _Sequence>& __it) noexcept + -> decltype(std::__to_address(__it.base().base())) + { return std::__to_address(__it.base().base()); } + +_GLIBCXX_END_NAMESPACE_VERSION +} +#endif + #undef _GLIBCXX_DEBUG_VERIFY_DIST_OPERANDS #undef _GLIBCXX_DEBUG_VERIFY_REL_OPERANDS #undef _GLIBCXX_DEBUG_VERIFY_EQ_OPERANDS diff --git a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc index 510d627435f..6afc6540609 100644 --- a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc +++ b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc @@ -1,6 +1,19 @@ // { dg-do compile { target { c++11 } } } #include +#include #include -char* p = std::__to_address(std::string("1").begin()); -const char* q = std::__to_address(std::string("2").cbegin()); +#include + +char* p __attribute__((unused)) + = std::__to_address(std::string("1").begin()); +const char* q __attribute__((unused)) + = std::__to_address(std::string("2").cbegin()); +int* r __attribute__((unused)) + = std::__to_address(std::vector(1, 1).begin()); +const int* s __attribute__((unused)) + = std::__to_address(std::vector(1, 1).cbegin()); +int* t __attribute__((unused)) + = std::__to_address(std::vector>(1, 1).begin()); +const int* u __attribute__((unused)) + = std::__to_address(std::vector>(1, 1).cbegin()); --------------ECF552303D1F419634C5DF99--