From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 8E9043857C78 for ; Fri, 30 Apr 2021 22:51:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8E9043857C78 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-100-Ay3CWtYNN-OXoasBBgc5kQ-1; Fri, 30 Apr 2021 18:51:25 -0400 X-MC-Unique: Ay3CWtYNN-OXoasBBgc5kQ-1 Received: by mail-qk1-f197.google.com with SMTP id h190-20020a3785c70000b02902e022511825so31423520qkd.7 for ; Fri, 30 Apr 2021 15:51:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=g8rJHBDOvr88qck6G2tbwOfPmwLm67OUTY9E2bBxk6s=; b=R90WpDkwJ+1DMCG1z6qm/7WZMM6Zp609xklZuXyudJgrII0RvV308dePBadZxVs2Vr kXXzTjT2e2s8tiSyh8M7d0JYa2dOTKO0lAXAlOJshPByZjTF1pfxoU1u/5g+jJaS8+aA tV55hFAkibrWv7fakWtBWOFovblsjrNTBKMMepMVexpBHmgMZQ3BWuOthM5DQyV75cy5 V0CvrgFato0XUNcOz4TZArXjPoY97sFKPxJwS5RokuUM1LyQiusstanZzSaIxIoxb4AB 89wfqMHb9T2yg8t3H9pjWvYpOoLXB0xHJWhuFo266Qx+sNyKXjDof6HjWxpMQi0L+gyK 6MsA== X-Gm-Message-State: AOAM533ITcWHLjcE6eFQFLvX8PGHEIApEjWiuHTWXjgMbifkAxAdVwo7 onnKsjdK6/TfB/EbrxALlizt5NLhGa/bBIZjY3ERY3JzmxsB/Ee9IJL7Ch0qmDu0DopWntxwSjX pMgBNYpUP73Gll1E= X-Received: by 2002:a05:620a:947:: with SMTP id w7mr8197551qkw.208.1619823084690; Fri, 30 Apr 2021 15:51:24 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwR24Q5eFT2lHUGINZXCn6rlokoDyupg3NIkGAovWFQw3TBkBPuEoyIJZpEEDuYx48LpvueSg== X-Received: by 2002:a05:620a:947:: with SMTP id w7mr8197527qkw.208.1619823084358; Fri, 30 Apr 2021 15:51:24 -0700 (PDT) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id e28sm3494680qtg.24.2021.04.30.15.51.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 30 Apr 2021 15:51:23 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 30 Apr 2021 18:51:23 -0400 (EDT) To: Jonathan Wakely cc: Patrick Palka , Tim Song , libstdc++ , gcc-patches Subject: Re: [PATCH] libstdc++: Implement P2328 changes to join_view In-Reply-To: <20210430220114.GE3008@redhat.com> Message-ID: <8322769f-0ea-c1f9-b5e4-1170defb06@idea> References: <20210430143835.893089-1-ppalka@redhat.com> <21f9ca23-18bb-fafe-4028-bd5fc1f4d3c@idea> <20210430220114.GE3008@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-15.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Fri, 30 Apr 2021 22:51:29 -0000 On Fri, 30 Apr 2021, Jonathan Wakely wrote: > On 30/04/21 17:34 -0400, Patrick Palka via Libstdc++ wrote: > > On Fri, 30 Apr 2021, Tim Song wrote: > > > > > On Fri, Apr 30, 2021 at 12:11 PM Patrick Palka via Libstdc++ > > > wrote: > > > > > > > > + template > > > > + _Tp& > > > > + _M_emplace_deref(const _Iter& __i) > > > > + { > > > > + this->reset(); > > > > + return this->emplace(*__i); > > > > + } > > > > > > This incurs a move, avoiding of which is the sole reason for > > > emplace-deref's existence. > > > > Ah thanks, I had missed that... IIUC, if we instead derive from > > optional's internal base class _Optional_base, we can easily get at the > > underlying storage for the object and directly initialize it with '*__i' > > and avoid the move. How does the the following adjustment to the patch > > look? > > Looks good to me, thanks. Here's what I ended up committing, complete with a comment to __non_propagating_cache::_M_emplace_deref and a test to verify we're avoiding the extra move: -- >8 -- Subject: [PATCH] libstdc++: Implement P2328 changes to join_view This implements the wording changes of P2328R0 "join_view should join all views of ranges". libstdc++-v3/ChangeLog: * include/std/ranges (__detail::__non_propating_cache): Define as per P2328. (join_view): Remove constraints on the value and reference types of the wrapped iterator type as per P2328. (join_view::_Iterator::_M_satisfy): Adjust as per P2328. (join_view::_Iterator::operator++): Likewise. (join_view::_M_inner): Use __non_propating_cache as per P2328. Remove now-redundant use of __maybe_present_t. * testsuite/std/ranges/adaptors/join.cc: Include . (test10): New test. --- libstdc++-v3/include/std/ranges | 77 ++++++++++++++++--- .../testsuite/std/ranges/adaptors/join.cc | 24 ++++++ 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges index 4975d5c630b..4a7ca49b45d 100644 --- a/libstdc++-v3/include/std/ranges +++ b/libstdc++-v3/include/std/ranges @@ -2241,10 +2241,68 @@ namespace views::__adaptor inline constexpr _DropWhile drop_while; } // namespace views + namespace __detail + { + template + struct __non_propagating_cache + { + // When _Tp is not an object type (e.g. is a reference type), we make + // __non_propagating_cache<_Tp> empty rather than ill-formed so that + // users can easily conditionally declare data members with this type + // (such as join_view::_M_inner). + }; + + template + requires is_object_v<_Tp> + struct __non_propagating_cache<_Tp> : private _Optional_base<_Tp> + { + __non_propagating_cache() = default; + + constexpr + __non_propagating_cache(const __non_propagating_cache&) noexcept + { } + + constexpr + __non_propagating_cache(__non_propagating_cache&& __other) noexcept + { __other._M_reset(); } + + constexpr __non_propagating_cache& + operator=(const __non_propagating_cache& __other) noexcept + { + if (std::__addressof(__other) != this) + this->_M_reset(); + return *this; + } + + constexpr __non_propagating_cache& + operator=(__non_propagating_cache&& __other) noexcept + { + this->_M_reset(); + __other._M_reset(); + return *this; + } + + constexpr _Tp& + operator*() noexcept + { return this->_M_get(); } + + template + _Tp& + _M_emplace_deref(const _Iter& __i) + { + this->_M_reset(); + // Using _Optional_base::_M_construct to initialize from '*__i' + // would incur an extra move due to the indirection, so we instead + // use placement new directly. + ::new ((void *) std::__addressof(this->_M_payload._M_payload)) _Tp(*__i); + this->_M_payload._M_engaged = true; + return this->_M_get(); + } + }; + } + template requires view<_Vp> && input_range> - && (is_reference_v> - || view>) class join_view : public view_interface> { private: @@ -2310,17 +2368,16 @@ namespace views::__adaptor constexpr void _M_satisfy() { - auto __update_inner = [this] (range_reference_t<_Base> __x) -> auto& - { + auto __update_inner = [this] (const iterator_t<_Base>& __x) -> auto&& { if constexpr (_S_ref_is_glvalue) - return __x; + return *__x; else - return (_M_parent->_M_inner = views::all(std::move(__x))); + return _M_parent->_M_inner._M_emplace_deref(__x); }; for (; _M_outer != ranges::end(_M_parent->_M_base); ++_M_outer) { - auto& __inner = __update_inner(*_M_outer); + auto&& __inner = __update_inner(_M_outer); _M_inner = ranges::begin(__inner); if (_M_inner != ranges::end(__inner)) return; @@ -2396,7 +2453,7 @@ namespace views::__adaptor if constexpr (_S_ref_is_glvalue) return *_M_outer; else - return _M_parent->_M_inner; + return *_M_parent->_M_inner; }(); if (++_M_inner == ranges::end(__inner_range)) { @@ -2507,10 +2564,8 @@ namespace views::__adaptor friend _Sentinel; }; - // XXX: _M_inner is "present only when !is_reference_v<_InnerRange>" [[no_unique_address]] - __detail::__maybe_present_t, - views::all_t<_InnerRange>> _M_inner; + __detail::__non_propagating_cache> _M_inner; _Vp _M_base = _Vp(); public: diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/join.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/join.cc index e6c71d771de..d774e8d9385 100644 --- a/libstdc++-v3/testsuite/std/ranges/adaptors/join.cc +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/join.cc @@ -19,6 +19,7 @@ // { dg-do run { target c++2a } } #include +#include #include #include #include @@ -170,6 +171,28 @@ test10() VERIFY( ranges::next(v.begin()) == v.end() ); } +void +test11() +{ + // Verify P2328 changes. + int r[] = {1, 2, 3}; + auto v = r + | views::transform([] (int n) { return std::vector{{n, -n}}; }) + | views::join; + VERIFY( ranges::equal(v, (int[]){1, -1, 2, -2, 3, -3}) ); + + struct S { + S() = default; + S(const S&) = delete; + S(S&&) = delete; + }; + auto w = r + | views::transform([] (int) { return std::array{}; }) + | views::join; + for (auto& i : w) + ; +} + int main() { @@ -183,4 +206,5 @@ main() test08(); test09(); test10(); + test11(); } -- 2.31.1.442.g7e39198978