From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6408 invoked by alias); 8 Jan 2018 13:37:08 -0000 Mailing-List: contact libstdc++-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libstdc++-owner@gcc.gnu.org Received: (qmail 6366 invoked by uid 89); 8 Jan 2018 13:37:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=8.cc, UD:8.cc, 8cc, season X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Jan 2018 13:36:56 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7C8F768572; Mon, 8 Jan 2018 13:36:55 +0000 (UTC) Received: from localhost (unknown [10.33.36.88]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2D1CC19156; Mon, 8 Jan 2018 13:36:55 +0000 (UTC) Date: Mon, 08 Jan 2018 13:37:00 -0000 From: Jonathan Wakely To: Ville Voutilainen Cc: gcc-patches@gcc.gnu.org, libstdc++ Subject: Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable} Message-ID: <20180108133654.GA23142@redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.1 (2017-09-22) X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00036.txt.bz2 On 25/12/17 23:59 +0200, Ville Voutilainen wrote: >In the midst of the holiday season, the king and ruler of all elves, otherwise >known as The Elf, was told by little elves that users are complaining how >stlstl and libc++ make optional's copy and move operations conditionally >trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he spoke >"this will not stand". > >Tested on Linux-PPC64. The change is an ABI break due to changing >optional to a trivially copyable type. It's perhaps >better to get that ABI break in now rather than later. Agreed, but a few comments and questions below. >@@ -203,6 +200,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > this->_M_construct(std::move(__other._M_payload)); > } > >+ _Optional_payload& >+ operator=(const _Optional_payload& __other) >+ { >+ if (this->_M_engaged && __other._M_engaged) >+ this->_M_get() = __other._M_get(); >+ else >+ { >+ if (__other._M_engaged) >+ this->_M_construct(__other._M_get()); >+ else >+ this->_M_reset(); >+ } >+ >+ return *this; >+ } >+ >+ _Optional_payload& >+ operator=(_Optional_payload&& __other) >+ noexcept(__and_, >+ is_nothrow_move_assignable<_Tp>>()) >+ { >+ if (this->_M_engaged && __other._M_engaged) >+ this->_M_get() = std::move(__other._M_get()); >+ else >+ { >+ if (__other._M_engaged) >+ this->_M_construct(std::move(__other._M_get())); >+ else >+ this->_M_reset(); >+ } >+ return *this; >+ } Please make the whitespace before the return statement consistent in these two assignment operators (one has a blank line and uses spaces, one uses a tab). >@@ -226,95 +256,86 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _Stored_type(std::forward<_Args>(__args)...); > this->_M_engaged = true; > } >- }; >- >- // Payload for non-constexpr optionals with trivial destructor. >- template >- struct _Optional_payload<_Tp, false, true> >- { >- constexpr _Optional_payload() >- : _M_empty() {} >- >- template >- constexpr _Optional_payload(in_place_t, _Args&&... __args) >- : _M_payload(std::forward<_Args>(__args)...), >- _M_engaged(true) {} >- >- template >- constexpr _Optional_payload(std::initializer_list<_Up> __il, >- _Args&&... __args) >- : _M_payload(__il, std::forward<_Args>(__args)...), >- _M_engaged(true) {} >- constexpr >- _Optional_payload(bool __engaged, const _Optional_payload& __other) >- : _Optional_payload(__other) >- {} > >- constexpr >- _Optional_payload(bool __engaged, _Optional_payload&& __other) >- : _Optional_payload(std::move(__other)) >- {} >+ // The _M_get operations have _M_engaged as a precondition. >+ constexpr _Tp& >+ _M_get() noexcept >+ { >+ return this->_M_payload; >+ } > >- constexpr _Optional_payload(const _Optional_payload& __other) >+ constexpr const _Tp& >+ _M_get() const noexcept > { >- if (__other._M_engaged) >- this->_M_construct(__other._M_payload); >+ return this->_M_payload; > } > >- constexpr _Optional_payload(_Optional_payload&& __other) >+ // _M_reset is a 'safe' operation with no precondition. >+ void >+ _M_reset() Should this be noexcept? > { >- if (__other._M_engaged) >- this->_M_construct(std::move(__other._M_payload)); >+ if (this->_M_engaged) >+ { >+ this->_M_engaged = false; >+ this->_M_payload.~_Stored_type(); >+ } > } >+ }; This closing brace seems to be indented incorrectly. >- using _Stored_type = remove_const_t<_Tp>; >- struct _Empty_byte { }; >- union { >- _Empty_byte _M_empty; >- _Stored_type _M_payload; >- }; >- bool _M_engaged = false; >+ template >+ class _Optional_base_impl >+ { >+ protected: And thos whole class body should be indented to line up with the "class" keyword. >+ using _Stored_type = remove_const_t<_Tp>; >+ >+ // The _M_construct operation has !_M_engaged as a precondition >+ // while _M_destruct has _M_engaged as a precondition. >+ template >+ void >+ _M_construct(_Args&&... __args) >+ noexcept(is_nothrow_constructible<_Stored_type, _Args...>()) >+ { >+ ::new >+ (std::__addressof(static_cast<_Dp*>(this)->_M_payload._M_payload)) >+ _Stored_type(std::forward<_Args>(__args)...); >+ static_cast<_Dp*>(this)->_M_payload._M_engaged = true; >+ } > >- template >- void >- _M_construct(_Args&&... __args) >- noexcept(is_nothrow_constructible<_Stored_type, _Args...>()) >- { >- ::new ((void *) std::__addressof(this->_M_payload)) >- _Stored_type(std::forward<_Args>(__args)...); >- this->_M_engaged = true; >- } >- }; >+ void >+ _M_destruct() noexcept? >+ { >+ static_cast<_Dp*>(this)->_M_payload._M_engaged = false; >+ static_cast<_Dp*>(this)->_M_payload._M_payload.~_Stored_type(); >+ } >+ >+ // _M_reset is a 'safe' operation with no precondition. >+ void >+ _M_reset() noexcept? >+ template+ bool = is_trivially_copy_constructible_v<_Tp>, >+ bool = is_trivially_move_constructible_v<_Tp>> > class _Optional_base >+ : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> Is "protected" inheritance the right choice? (Is protected inheritance *ever* the right choice?) Everything in the base could be public, and then just use private inheritance. >+ // Copy and move constructors. >+ constexpr _Optional_base(const _Optional_base& __other) >+ = default; Please put the line break after "constexpr" instead. >+ // Copy and move constructors. >+ constexpr _Optional_base(const _Optional_base& __other) >+ = default; Same here. >diff --git a/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc >new file mode 100644 >index 0000000..c00428e >--- /dev/null >+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc >@@ -0,0 +1,101 @@ >+// { dg-options "-std=gnu++17" } >+// { dg-do compile } >+ >+// Copyright (C) 2013-2017 Free Software Foundation, Inc. >diff --git a/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc b/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc >new file mode 100644 >index 0000000..541158e >--- /dev/null >+++ b/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc >@@ -0,0 +1,47 @@ >+// { dg-options "-std=gnu++17" } >+// { dg-do compile } >+ >+// Copyright (C) 2013-2017 Free Software Foundation, Inc. These new tests should just have a copyright date of 2018.