From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x432.google.com (mail-wr1-x432.google.com [IPv6:2a00:1450:4864:20::432]) by sourceware.org (Postfix) with ESMTPS id AAAE4385841D for ; Thu, 7 Oct 2021 18:34:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AAAE4385841D Received: by mail-wr1-x432.google.com with SMTP id r10so21926500wra.12 for ; Thu, 07 Oct 2021 11:34: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:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EHDOGswRhJNs009+m0W6v4D/MByXQ4fSUc85QpkpMfI=; b=CiSSI2oD3Vs3vwMsNA7I8NvaxZWJGxDeiombfCMamKXmFyP7u19mYeybYSbS8wrvKJ hVG4V2DawHnnpAYjhQdkSyQ8jgangU+LpPlhoG7Kte8e8hVFRhjf8W4hWJDjb7elkYPo wQR8A66t0I4TRRyBzr9D8qLOw/O/qN7udfiwZ4zPm3ugFIkfUQR462hf31/ZXeBuQCJv HeV8rI0RjNjiAdC2/3KLpoWxPJ+emqWfdE5aThb9vM/LksMDRQsZRU2t4nTnVL7wq6dZ l4RsuuAYRQ2IcceEyqq0k/EdMeXO6asvXcRh70EtNFZv92/Ud39yBluOWNo3QBfRYQEk V6rA== X-Gm-Message-State: AOAM530DuFsBInEusnUW3bp9NwTmtQX+APw6Zi+OjwFnu0u2eh3u+v7n Cy0xAuSwy/BgssTN78UGqzn8XFtgaMxJsJFvoS0= X-Google-Smtp-Source: ABdhPJwWB9giy43+FXu1gPs3a0OB1+iKwL0hTztpkCVfBt3c6rcefUqw1Aeh238rwt9G6+5GFugLW6P6JeqhizW2OlA= X-Received: by 2002:a5d:64ab:: with SMTP id m11mr7293403wrp.343.1633631659780; Thu, 07 Oct 2021 11:34:19 -0700 (PDT) MIME-Version: 1.0 References: <237efbd9-abd4-47e9-0af9-dace431d5da7@gmail.com> <7b5d47fe-39c8-c170-4037-d8d22733528f@gmail.com> <2cf058da-0136-ccad-5ffd-4719c0b04d03@gmail.com> In-Reply-To: <2cf058da-0136-ccad-5ffd-4719c0b04d03@gmail.com> From: Jonathan Wakely Date: Thu, 7 Oct 2021 19:34:08 +0100 Message-ID: Subject: Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Jonathan Wakely , "libstdc++" X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, HTML_MESSAGE, KAM_SHORT, 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" Content-Transfer-Encoding: quoted-printable 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 18:34:23 -0000 On Thu, 7 Oct 2021, 18:34 Fran=C3=A7ois Dumont, wrot= e: > On 07/10/21 7:16 pm, Jonathan Wakely wrote: > > On Thu, 7 Oct 2021 at 18:06, Fran=C3=A7ois 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=C3=A7ois Dumont, > wrote: > >>>> > >>>>> On 02/10/21 2:47 pm, Jonathan Wakely wrote: > >>>>> > >>>>> > >>>>> > >>>>> On Sat, 2 Oct 2021, 13:02 Fran=C3=A7ois 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_i= f. > >>>>>>> > >>>>>>> Also unwrap iterators using __niter_base, to avoid redundant debu= g > 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 yo= u > would > >>>>> be missing the std::__niter_wrap in the __cont.erase(__iter), it ju= st > >>>>> 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 solutio= n. > >>>>>> > >>>>> 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 the= y > 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 nod= e > >>>> 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 =3D __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 t= he > >> 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 betwe= n > >> 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 namespac= e. > > 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. > >> 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. B= ut > >> 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 pl= an > >> 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. > OK, I'll remove it.