From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x429.google.com (mail-wr1-x429.google.com [IPv6:2a00:1450:4864:20::429]) by sourceware.org (Postfix) with ESMTPS id 2C6ED385841D for ; Thu, 7 Oct 2021 19:51:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2C6ED385841D Received: by mail-wr1-x429.google.com with SMTP id r7so22459621wrc.10 for ; Thu, 07 Oct 2021 12:51:16 -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-language; bh=nY5p/lEJpgTtL3Dvx3CD62BkEgV8ndtPlH5EoRVO8U8=; b=amrt6CD6H8hpAxA9X6LajF9sfjvr2UZdyIb+dvvoHf8khNlPfqnyDm5PuwS950AUdw HjXGVegwci+xKPJWRvmHyJs45NLmoCplUnSugoHzMduWXn72rbwkqaHGRze3m5Buk0Tn sEM6HjaouhuiFcodcse0uN1eQQaIRuBJTyPU7g03971kzETFbVnCICUztPlgJmyDfIKM 503VT4txXcrEaQtofmjK1Qcka/8eFq9pLEY2TO266x5JqtYzyAXVz14Fx2Uur60Txg/K ncuYdyAJMOLT1mR/vbc07fiWeU2pgOftpInlJi62juA75/LsQ+kGvubh5H5zZ9kOtacb KHeA== X-Gm-Message-State: AOAM531egYqRaIVJIy+qKBcHCU3u7fU9GICOg3qzuEnw1fcMF47/RR+y YW8yA82snEZX5T3yGxKm+JbKzIBjcJ0MUg== X-Google-Smtp-Source: ABdhPJxWmpDroEn1pHdXRfQuNZOom3U1csCMGL9IqIJMoKy5wH25l/kpVcK4CgLXj/2Se2nyPcO9wA== X-Received: by 2002:a05:6000:162f:: with SMTP id v15mr8135840wrb.118.1633636274780; Thu, 07 Oct 2021 12:51:14 -0700 (PDT) Received: from ?IPv6:2a01:e0a:1dc:b1c0:88b3:8b08:7ca0:b5f6? ([2a01:e0a:1dc:b1c0:88b3:8b08:7ca0:b5f6]) by smtp.googlemail.com with ESMTPSA id w11sm295324wmc.44.2021.10.07.12.51.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Oct 2021 12:51:14 -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> <2cf058da-0136-ccad-5ffd-4719c0b04d03@gmail.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <82a15e95-a28d-2ffe-b5da-965f8b7242b6@gmail.com> Date: Thu, 7 Oct 2021 21:51:12 +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-Language: en-US X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, HTML_MESSAGE, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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 19:51:18 -0000 On 07/10/21 8:34 pm, Jonathan Wakely wrote: > > > On Thu, 7 Oct 2021, 18:34 François Dumont, > wrote: > > 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 ? > > > I'm undecided. One the one hand, having the public base breaks > encapsulation, but on the other hand it's a documented part of the API. > > It would be good to know how many users actually rely on it, before we > change the guarantees. Don't worry, I won't do it. We live with that until now and moreover I am planing to do something like this:     template       typename _Container::size_type       __erase_nodes_if(_Container& __cont, _UnsafeContainer& __ucont, _Predicate __pred) and in the implementation we will do something like: __cont.erase(__ucont.begin()); So we'll need the inheritance. People will just have to be careful, I'll check that documentation is clear on that.