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

["François Dumont" <frs.dumont@gmail.com>]

On 07/10/21 7:16 pm, Jonathan Wakely wrote:
> 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.

Indeed, it just allowed me to spot for example that the 'using 
_Base::merge' in the safe unordered containers is wrong because it does 
what you intended to do here that is to say modify underlying container 
in the back of the safe one. I am fixing this.


>
> 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.

Well, I can change the doc :-)

Do you confirm that you don't want to change this ?

>> 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.
>
Yes, for now I think it is better to remove any safe-container 
consideration in this code.