From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) by sourceware.org (Postfix) with ESMTPS id 5D5BA3858018 for ; Thu, 7 Oct 2021 17:06:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5D5BA3858018 Received: by mail-ed1-x52d.google.com with SMTP id t16so3852633eds.9 for ; Thu, 07 Oct 2021 10:06:20 -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=Qb4gJjgqtIMiB3FVQVNrdBO6tDaF9y0+pjA0Rzz2VIo=; b=3I/v+w9jAddGzRGeA/wTCk8xsRni2bGIkA0M8rKnvo7QmFy1WuJlz5dnJ4Ys1eqMDg ynvbbEBs/ciFyXC4bAFiVNIN1Q+bS/2uSNTtwqz3L47Xlap/O1jr967YcskDg/fpyW2E hC9NtKYuPCIpz+usN9+FWOGkagbTEYkizG0ZawT62QDkEeDvXrjOujW2HhM0ZYmNiV22 J90ENN0MZzXnI6+f3buMSw64wLD/yS0PHb+BYU8giGYf+3o7eNQrVG2ggp8mK1ZVeNx2 GtAKLlLNzQpEH3ijCl/cSf4NWr03tAJZGtfbNIfaLsTxuCecRZXgxiqqxgs6tGtI+gwo cUvw== X-Gm-Message-State: AOAM5309BOkPF0VnjmsDnOdyrzjQXpbp4eWei5PICIlaH7zwu20HUcX3 eH06wOp6MOnVqxzcLZoxOaWx51YUDCc= X-Google-Smtp-Source: ABdhPJyqTMrCyXzv+9vGRIDxZIuMcEvr3oC5D59QO4wvioXdOvOYin2UkeWSVhAS4LjhYF7fTxtWpQ== X-Received: by 2002:a05:6402:5193:: with SMTP id q19mr7813316edd.397.1633626378970; Thu, 07 Oct 2021 10:06:18 -0700 (PDT) Received: from [10.34.5.219] ([109.190.253.15]) by smtp.googlemail.com with ESMTPSA id r22sm64631ejj.91.2021.10.07.10.06.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Oct 2021 10:06:18 -0700 (PDT) Subject: Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] To: Jonathan Wakely , Jonathan Wakely Cc: libstdc++ References: <237efbd9-abd4-47e9-0af9-dace431d5da7@gmail.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <7b5d47fe-39c8-c170-4037-d8d22733528f@gmail.com> Date: Thu, 7 Oct 2021 19:06:16 +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.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, 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:06:23 -0000 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. 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 ? 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.