Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely <jwakely@redhat.com>]

On Thu, 7 Oct 2021 at 18:06, François Dumont <frs.dumont@gmail.com> wrote:
>
> On 07/10/21 4:02 pm, Jonathan Wakely wrote:
> > On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
> >> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:
> >>
> >>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
> >>>
> >>>
> >>>
> >>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
> >>> libstdc++@gcc.gnu.org> wrote:
> >>>
> >>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> >>>>> This reduces the preprocessed size of <deque>, <string> and <vector> by
> >>>>> not including <bits/stl_algo.h> for std::remove and std::remove_if.
> >>>>>
> >>>>> Also unwrap iterators using __niter_base, to avoid redundant debug mode
> >>>>> checks.
> >>>> I don't know if you noticed but the __niter_base is a no-op here.
> >>>>
> >>> Oh, I didn't notice.
> >>>
> >>>
> >>>> __niter_base unwrap only random access iterators because it is the only
> >>>> category for which we know that we have been able to confirm validity or
> >>>> not.
> >>>>
> >>> But these are all random access. I must be missing something.
> >>>
> >>> It is in a method called '__erases_node_if', I'm not aware of any
> >>> node-based container providing random access iterators.
> >>>
> >> Ah, I thought you meant the deque, string and vector part of the patch,
> >> sorry.
> >>
> >> Moreover I just noticed that if it was really doing anything then you would
> >>> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
> >>> wouldn't compile.
> >>>
> >>>
> >>>> We would need to introduce another function to do this or specialize in
> >>>> some way erase_if for debug containers. I'll try to find a solution.
> >>>>
> >>> OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
> >>> the overhead because we know that the iterator range is valid. Any checks
> >>> done on each increment and equality comparison are wasteful, as they will
> >>> never fail.
> >>>
> >>> Yes, that would be better indeed.
> >>>
> >>> But doing it this way you still have the overhead of the _Safe_iterator
> >>> addition to the list of the safe container iterators, so a mutex
> >>> lock/unlock.
> >>>
> >>> I'll try to find out how to get a normal iterator from a safe container
> >>> even if in this case we will have to support operations on safe container
> >>> with normal iterators.
> >>>
> >> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
> >> Alloc>::begin() to access the base version directly. But for the node
> >> containers we have a generic function where we don't know the exact type.
> >> Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
> >> would work, but it's ugly.
> > The solution is simple:
> >
> >      erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
> > -    { return __detail::__erase_nodes_if(__cont, __pred); }
> > +    {
> > +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
> > +      return __detail::__erase_nodes_if(__c, __pred);
> > +    }
> >
> >
> > i.e. just bind a reference to the non-debug container type. For
> > non-debug mode, that is a no-op. For debug mode it binds to the base,
> > and the rest of the function works directly on the base, without safe
> > iterators.
> >
> > I'm testing the patch now.
> >
> Yes, it's a nice approach.
>
> But the problem is that you are going to potentially erase node from the
> container in the back of the safe container. In the end we might have
> valid _Safe_iterator pointing to destroyed nodes.

Bah. You're right, unfortunately I just pushed the patch.

>
> In fact I am working on a patch to remove the public inheritance betwen
> the Safe container and its normal counterpart to break this kind of
> code. Do you think it would be ok ?

It might break valid uses of the containers in the __gnu_debug namespace.

We document at https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html
that the real container is a public base class of the debug one.


>
> Ideally I would have like to allow a user to access a const reference to
> the underlying "unsafe" container, but I don't think C++ allow this. But
> for your code it would still be considered as invalid.
>
> Here what we need is to get an unsafe iterator from the safe-container
> and pass it back to the safe-container for deletion. This is what I plan
> to work on after getting rid of the public inheritance.

Or maybe we should just leave the debug checks in place, and live with
the overhead. That seems simplest. If we can't remove the overhead
entirely, then we should just let the safe containers correctly track
the changes to the containers. And the simplest way is to just use the
normal interface of the debug containers. Otherwise those very simple
functions get complicated and hard to understand.