From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id 619913858413 for ; Thu, 7 Oct 2021 17:34:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 619913858413 Received: by mail-ed1-x532.google.com with SMTP id g8so25948352edt.7 for ; Thu, 07 Oct 2021 10:34:04 -0700 (PDT) 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-transfer-encoding :content-language; bh=6YIO3BeeQDcJnYXXlXozaPZVTSYLQongog/TEK2LfV8=; b=QjNKaVeRyHgpDxU7Y8jGt5C4c5YjicgFzaRrh6CtAaCk4MVKL9LKv1ghiF8kWp55AD 5OP9GDTrPaYFbSN0ETMKdbMr6oj9h9UdfDDUfJ2rnmpgsZsytyZXuUEd4N0/QCI7ODYC xRIzOr2XoC44GINgnzxoN9azFoDDFSZpBIre9zWgCSF9b9Q8ZEnABk0qzYEeao9iPkAg gGUIHMUOtnIcljmoXkrfQTLCQC+nchdr4FUnndgEoZoS1m/5GkFzeRQznzZg3JlI9zJb U0nZNoztWxGqqhYOv8+mMIB8NiQFMYTSr4+nz/7ujgIPLMuibttdy6ggYbTftKuX1J0X 33iQ== X-Gm-Message-State: AOAM531fnOP+NpN4HZAXvelGgFK1uLDXBNgL1KgrQlmTGZbdo6N6EPOn iMs5UUL7gejCfmgWrpLtyoeGQ814NRs= X-Google-Smtp-Source: ABdhPJwsz+ZXX3OgIBEebCd+xBl+MZtx9FyUjgIWkBElEqdY94uIjQTaY1E657OqVTwAEx0m7bNRmQ== X-Received: by 2002:a17:906:160a:: with SMTP id m10mr7090457ejd.259.1633628043245; Thu, 07 Oct 2021 10:34:03 -0700 (PDT) Received: from [10.34.5.219] ([109.190.253.15]) by smtp.googlemail.com with ESMTPSA id z4sm39538edd.46.2021.10.07.10.34.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Oct 2021 10:34:02 -0700 (PDT) Subject: Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] To: Jonathan Wakely Cc: Jonathan Wakely , libstdc++ References: <237efbd9-abd4-47e9-0af9-dace431d5da7@gmail.com> <7b5d47fe-39c8-c170-4037-d8d22733528f@gmail.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <2cf058da-0136-ccad-5ffd-4719c0b04d03@gmail.com> Date: Thu, 7 Oct 2021 19:34:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: fr X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Thu, 07 Oct 2021 17:34:06 -0000 On 07/10/21 7:16 pm, Jonathan Wakely wrote: > On Thu, 7 Oct 2021 at 18:06, François Dumont wrote: >> On 07/10/21 4:02 pm, Jonathan Wakely wrote: >>> On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++ >>> wrote: >>>> On Thu, 7 Oct 2021, 07:35 François Dumont, 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 , and by >>>>>>> not including 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>>> 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.