public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jonathan Wakely <jwakely@redhat.com>
To: "François Dumont" <frs.dumont@gmail.com>
Cc: Jonathan Wakely <jwakely.gcc@gmail.com>,
	"libstdc++" <libstdc++@gcc.gnu.org>,
	gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>>
Date: Wed, 15 Dec 2021 21:21:10 +0000	[thread overview]
Message-ID: <CACb0b4kU1sb1paWg55=Q4itgysmOoX93sUXaAH1CWiPqfo=bQg@mail.gmail.com> (raw)
In-Reply-To: <7a748ec5-b0b7-fb4d-312e-4a17b6bac8fa@gmail.com>

On Wed, 15 Dec 2021 at 21:16, François Dumont <frs.dumont@gmail.com> wrote:

> 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<P, C>> fails to rebind C, so you get
> incorrect types
>     like __normal_iterator<long*, vector<int>>. 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ç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<typename _Ptr, typename... _None>
>> >     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 <regex> and <filesystem>, 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<P, C>> fails to
> rebind C, so you get incorrect types like __normal_iterator<long*,
> vector<int>>.
>
>
> >>
>> >>     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 <bits/ptr_traits.h>.
>
> 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<int, __gnu_test::CustomPointerAlloc<int>>.
>
> OK with those changes, thanks.
>
>
>
>
>

      reply	other threads:[~2021-12-15 21:21 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-28 19:25 [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> Jonathan Wakely
2021-09-30 20:24 ` François Dumont
2021-10-01 22:29   ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>> Jonathan Wakely
2021-10-02 13:08     ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> François Dumont
2021-10-02 20:28       ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>> Jonathan Wakely
2021-10-02 17:27     ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> François Dumont
2021-10-02 20:24       ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>> Jonathan Wakely
2021-10-04 20:05         ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> François Dumont
2021-10-04 20:26           ` François Dumont
2021-10-04 20:30             ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>> Jonathan Wakely
2021-10-06 17:18               ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> François Dumont
2021-10-06 17:25                 ` François Dumont
2021-12-14  6:53                   ` François Dumont
2021-12-14 13:12                     ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>> Jonathan Wakely
2021-12-15 21:16                       ` [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> François Dumont
2021-12-15 21:21                         ` Jonathan Wakely [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACb0b4kU1sb1paWg55=Q4itgysmOoX93sUXaAH1CWiPqfo=bQg@mail.gmail.com' \
    --to=jwakely@redhat.com \
    --cc=frs.dumont@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jwakely.gcc@gmail.com \
    --cc=libstdc++@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).