From patchwork Mon May 29 20:55:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Dumont?= X-Patchwork-Id: 768353 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wc8Cc6jTNz9s72 for ; Tue, 30 May 2017 06:55:56 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="dDr3uVDg"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=T6z0HhTH/3n+jdvKe L5oEpbKPSoBA1WdB2JUdWeOEUrZZ2o+hwb5SciE+h+LjZ6jRlHvoZqMdTE5r8KQi wmDSXIS/a551vrtgrd869cUp0LacV1N++zg9yZaNVYoktWqki9d9js8SBKLmH6cJ 8BilexDQMrBVrfKKu1zldpRloY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=Oq1D1OMoNETDZA+vD4a0dsX WOcY=; b=dDr3uVDgZ7A61a8lCHDW8GUJYF8AmPzPCly0XJgmqJpVmcCD9ut94OT a7oN6GJLGwQgNziSxZhtpInh1bnA3QqWVLqy+R7nZ/Xl/hM0f41QKCwCJnK8qMky S0GCoBuvF6X7wjm+Bfa06dPgRz9wR1XirJoVPJbNMUiyBk4AW4WM= Received: (qmail 65755 invoked by alias); 29 May 2017 20:55:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 65589 invoked by uid 89); 29 May 2017 20:55:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=11417, 7817 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wm0-f51.google.com Received: from mail-wm0-f51.google.com (HELO mail-wm0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 29 May 2017 20:55:36 +0000 Received: by mail-wm0-f51.google.com with SMTP id 7so68230351wmo.1; Mon, 29 May 2017 13:55:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=/l6UmiBy3/OMsO7i4u+p6mTWyCwNL4M8VJwK3jj77c8=; b=SfxyhSKH/13Wg2Hph7k4g9cwJsMeSyEE8Z1hwKeLfxqyzoCN9fTB10YD8DBw19XmFM 0DfBTXGq0FzdSAcqVclE1OE+vnWjgp/R1kScW7xVtP0jCmPary3ufknq0rv2YpitxblV uGs4haxfFCQx9txLhC7Qv8ML7HvI9RUnhdFFTaDV9xp4bCqsXyVJ5N7chGwkDQFNj3Bj fd7A3gjgIFm3Som/AiLV12fs4zm+z8I53EKdMVJ3q8mvj8DLdDJSfYaDFXJd093LgBBx vMxqbnlUt78iouJtA5DEC51yeE8s5O/RMIGT/C20s5ty7EqAwIKAEeJ0JHuyDNHVhaiz Ezrw== X-Gm-Message-State: AODbwcAcZBsLuy8ry2m3ZR6lG62afcqlX2WL1vePUOsbAQSr/shAOymb 8MRWhV+tT9TlQb9W X-Received: by 10.28.210.5 with SMTP id j5mr21377390wmg.56.1496091337877; Mon, 29 May 2017 13:55:37 -0700 (PDT) Received: from [192.168.0.23] (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by smtp.googlemail.com with ESMTPSA id k53sm11348944wrc.10.2017.05.29.13.55.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 29 May 2017 13:55:36 -0700 (PDT) Subject: Re: Default std::vector default and move constructor To: Jonathan Wakely References: <20170525162816.GD12306@redhat.com> <20170527111401.GH12306@redhat.com> Cc: "libstdc++@gcc.gnu.org" , gcc-patches From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <4fcb496e-08ed-85ef-0470-e4f5b0d816ca@gmail.com> Date: Mon, 29 May 2017 22:55:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Hi It wasn't such a big deal to restore value-init of the allocator. So here is the updated patch. I used: _Bvector_impl() _GLIBCXX_NOEXCEPT_IF( noexcept(_Bit_alloc_type()) ) rather than using is_nothrow_default_constructible. Any advantage in one approach or the other ? I'll complete testing and add a test on this value-initialization before commit if you agree. Tests still running but I'm pretty sure it will work the same. François On 28/05/2017 22:13, François Dumont wrote: > On 27/05/2017 13:14, Jonathan Wakely wrote: >> On 26/05/17 23:13 +0200, François Dumont wrote: >>> On 25/05/2017 18:28, Jonathan Wakely wrote: >>>> On 15/05/17 19:57 +0200, François Dumont wrote: >>>>> Hi >>>>> >>>>> Following what I have started on RbTree here is a patch to >>>>> default implementation of default and move constructors on >>>>> std::vector. >>>>> >>>>> As in _Rb_tree_impl the default allocator is not value >>>>> initialized anymore. We could add a small helper type arround the >>>>> allocator to do this value initialization per default. Should I do >>>>> so ? >>>> >>>> It's required to be value-initialized, so if your patch changes that >>>> then it's a problem. >>>> >>>> Did we decide it's OK to do that for RB-trees? Did we actually discuss >>>> that part of the r243379 changes? >>> >>> I remember a message pointing this issue but after the commit AFAIR. >>> I thought it was from Tim but I can't find it on the archive. >>> >>> What is the rational of this requirement ? I started working on a >>> type to do the allocator value initialization if there is no default >>> constructor but it seems quite complicated to do so. It is quite sad >>> that we can't fully benefit from this nice C++11 feature just >>> because of this requirement. If there is any initialization needed >>> it doesn't sound complicated to provide a default constructor. >> >> The standard says that the default constructor is: >> >> vector() : vector(Allocator()) { } >> >> That value-initializes the allocator. If the allocator type behaves >> differently for value-init and default-init (e.g. it has data members >> that are left uninitialized by default-init) then the difference >> matters. If you change the code so it only does default-init of the >> allocator then you will introduce an observable difference. >> >> I don't see any requirement that a DefaultConstructible allocator >> cannot leave members uninitialized, so that means the standard >> requires default construction of vector to value-init the >> allocator. Not default-init. > > Sure but like freedom which stop where start others' freedom so does > those requirements :-). Because the Standard says that an allocator > will be value-init when there is no default-init it makes usage of the > C++11 default constructor more complicated. > > But as it is unavoidable here is a type I tried to work on to keep > current implementations as long as we inherit from > __alloc_value_initializer. > > I don't like it myself but I propose just in case you are interested. > > Otherwise I am also going to rework my patch to keep this initialization. > > François > diff --git a/libstdc++-v3/include/bits/stl_bvector.h b/libstdc++-v3/include/bits/stl_bvector.h index 37e000a..6509ac5 100644 --- a/libstdc++-v3/include/bits/stl_bvector.h +++ b/libstdc++-v3/include/bits/stl_bvector.h @@ -388,10 +388,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER { return __x + __n; } inline void - __fill_bvector(_Bit_iterator __first, _Bit_iterator __last, bool __x) + __fill_bvector(_Bit_type * __v, + unsigned int __first, unsigned int __last, bool __x) { - for (; __first != __last; ++__first) - *__first = __x; + const _Bit_type __fmask = ~0ul << __first; + const _Bit_type __lmask = ~0ul >> (_S_word_bit - __last); + const _Bit_type __mask = __fmask & __lmask; + + if (__x) + *__v |= __mask; + else + *__v &= ~__mask; } inline void @@ -399,12 +406,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER { if (__first._M_p != __last._M_p) { - std::fill(__first._M_p + 1, __last._M_p, __x ? ~0 : 0); - __fill_bvector(__first, _Bit_iterator(__first._M_p + 1, 0), __x); - __fill_bvector(_Bit_iterator(__last._M_p, 0), __last, __x); + _Bit_type *__first_p = __first._M_p; + if (__first._M_offset != 0) + __fill_bvector(__first_p++, __first._M_offset, _S_word_bit, __x); + + __builtin_memset(__first_p, __x ? ~0 : 0, + (__last._M_p - __first_p) * sizeof(_Bit_type)); + + if (__last._M_offset != 0) + __fill_bvector(__last._M_p, 0, __last._M_offset, __x); } else - __fill_bvector(__first, __last, __x); + __fill_bvector(__first._M_p, __first._M_offset, __last._M_offset, __x); } template @@ -416,33 +429,61 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _Bit_alloc_traits; typedef typename _Bit_alloc_traits::pointer _Bit_pointer; - struct _Bvector_impl - : public _Bit_alloc_type + struct _Bvector_impl_data { _Bit_iterator _M_start; _Bit_iterator _M_finish; _Bit_pointer _M_end_of_storage; - _Bvector_impl() - : _Bit_alloc_type(), _M_start(), _M_finish(), _M_end_of_storage() + _Bvector_impl_data() _GLIBCXX_NOEXCEPT + : _M_start(), _M_finish(), _M_end_of_storage() + { } + +#if __cplusplus >= 201103L + _Bvector_impl_data(_Bvector_impl_data&& __x) noexcept + : _M_start(__x._M_start), _M_finish(__x._M_finish) + , _M_end_of_storage(__x._M_end_of_storage) + { __x._M_reset(); } + + void + _M_move_data(_Bvector_impl_data&& __x) noexcept + { + this->_M_start = __x._M_start; + this->_M_finish = __x._M_finish; + this->_M_end_of_storage = __x._M_end_of_storage; + __x._M_reset(); + } +#endif + + void + _M_reset() _GLIBCXX_NOEXCEPT + { + _M_start = _M_finish = _Bit_iterator(); + _M_end_of_storage = _Bit_pointer(); + } + }; + + struct _Bvector_impl + : public _Bit_alloc_type, public _Bvector_impl_data + { + public: + _Bvector_impl() _GLIBCXX_NOEXCEPT_IF( noexcept(_Bit_alloc_type()) ) + : _Bit_alloc_type() { } _Bvector_impl(const _Bit_alloc_type& __a) - : _Bit_alloc_type(__a), _M_start(), _M_finish(), _M_end_of_storage() + : _Bit_alloc_type(__a) { } #if __cplusplus >= 201103L - _Bvector_impl(_Bit_alloc_type&& __a) - : _Bit_alloc_type(std::move(__a)), _M_start(), _M_finish(), - _M_end_of_storage() - { } + _Bvector_impl(_Bvector_impl&&) = default; #endif _Bit_type* _M_end_addr() const _GLIBCXX_NOEXCEPT { - if (_M_end_of_storage) - return std::__addressof(_M_end_of_storage[-1]) + 1; + if (this->_M_end_of_storage) + return std::__addressof(this->_M_end_of_storage[-1]) + 1; return 0; } }; @@ -462,23 +503,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER get_allocator() const _GLIBCXX_NOEXCEPT { return allocator_type(_M_get_Bit_allocator()); } - _Bvector_base() - : _M_impl() { } +#if __cplusplus >= 201103L + _Bvector_base() = default; +#else + _Bvector_base() { } +#endif _Bvector_base(const allocator_type& __a) : _M_impl(__a) { } #if __cplusplus >= 201103L - _Bvector_base(_Bvector_base&& __x) noexcept - : _M_impl(std::move(__x._M_get_Bit_allocator())) - { - this->_M_impl._M_start = __x._M_impl._M_start; - this->_M_impl._M_finish = __x._M_impl._M_finish; - this->_M_impl._M_end_of_storage = __x._M_impl._M_end_of_storage; - __x._M_impl._M_start = _Bit_iterator(); - __x._M_impl._M_finish = _Bit_iterator(); - __x._M_impl._M_end_of_storage = nullptr; - } + _Bvector_base(_Bvector_base&&) = default; #endif ~_Bvector_base() @@ -500,11 +535,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _Bit_alloc_traits::deallocate(_M_impl, _M_impl._M_end_of_storage - __n, __n); - _M_impl._M_start = _M_impl._M_finish = _Bit_iterator(); - _M_impl._M_end_of_storage = _Bit_pointer(); + _M_impl._M_reset(); } } +#if __cplusplus >= 201103L + void + _M_move_data(_Bvector_base&& __x) noexcept + { _M_impl._M_move_data(std::move(__x._M_impl)); } +#endif + static size_t _S_nword(size_t __n) { return (__n + int(_S_word_bit) - 1) / int(_S_word_bit); } @@ -564,7 +604,8 @@ template typedef std::reverse_iterator reverse_iterator; typedef _Alloc allocator_type; - allocator_type get_allocator() const + allocator_type + get_allocator() const { return _Base::get_allocator(); } protected: @@ -574,11 +615,11 @@ template using _Base::_M_get_Bit_allocator; public: - vector() #if __cplusplus >= 201103L - noexcept(is_nothrow_default_constructible::value) + vector() = default; +#else + vector() { } #endif - : _Base() { } explicit vector(const allocator_type& __a) @@ -592,23 +633,16 @@ template vector(size_type __n, const bool& __value, const allocator_type& __a = allocator_type()) - : _Base(__a) - { - _M_initialize(__n); - std::fill(this->_M_impl._M_start._M_p, this->_M_impl._M_end_addr(), - __value ? ~0 : 0); - } #else explicit vector(size_type __n, const bool& __value = bool(), const allocator_type& __a = allocator_type()) +#endif : _Base(__a) { _M_initialize(__n); - std::fill(this->_M_impl._M_start._M_p, this->_M_impl._M_end_addr(), - __value ? ~0 : 0); + _M_initialize_value(__value); } -#endif vector(const vector& __x) : _Base(_Bit_alloc_traits::_S_select_on_copy(__x._M_get_Bit_allocator())) @@ -618,22 +652,14 @@ template } #if __cplusplus >= 201103L - vector(vector&& __x) noexcept - : _Base(std::move(__x)) { } + vector(vector&&) = default; vector(vector&& __x, const allocator_type& __a) noexcept(_Bit_alloc_traits::_S_always_equal()) : _Base(__a) { if (__x.get_allocator() == __a) - { - this->_M_impl._M_start = __x._M_impl._M_start; - this->_M_impl._M_finish = __x._M_impl._M_finish; - this->_M_impl._M_end_of_storage = __x._M_impl._M_end_of_storage; - __x._M_impl._M_start = _Bit_iterator(); - __x._M_impl._M_finish = _Bit_iterator(); - __x._M_impl._M_end_of_storage = nullptr; - } + this->_M_move_data(std::move(__x)); else { _M_initialize(__x.size()); @@ -716,12 +742,7 @@ template || this->_M_get_Bit_allocator() == __x._M_get_Bit_allocator()) { this->_M_deallocate(); - this->_M_impl._M_start = __x._M_impl._M_start; - this->_M_impl._M_finish = __x._M_impl._M_finish; - this->_M_impl._M_end_of_storage = __x._M_impl._M_end_of_storage; - __x._M_impl._M_start = _Bit_iterator(); - __x._M_impl._M_finish = _Bit_iterator(); - __x._M_impl._M_end_of_storage = nullptr; + this->_M_move_data(std::move(__x)); std::__alloc_on_move(_M_get_Bit_allocator(), __x._M_get_Bit_allocator()); } @@ -760,7 +781,7 @@ template typename = std::_RequireInputIter<_InputIterator>> void assign(_InputIterator __first, _InputIterator __last) - { _M_assign_dispatch(__first, __last, __false_type()); } + { _M_assign_aux(__first, __last, std::__iterator_category(__first)); } #else template void @@ -774,7 +795,7 @@ template #if __cplusplus >= 201103L void assign(initializer_list __l) - { this->assign(__l.begin(), __l.end()); } + { _M_assign_aux(__l.begin(), __l.end(), random_access_iterator_tag()); } #endif iterator @@ -1096,6 +1117,14 @@ template } void + _M_initialize_value(bool __x) + { + __builtin_memset(this->_M_impl._M_start._M_p, __x ? ~0 : 0, + (this->_M_impl._M_end_addr() - this->_M_impl._M_start._M_p) + * sizeof(_Bit_type)); + } + + void _M_reallocate(size_type __n); #if __cplusplus >= 201103L @@ -1112,8 +1141,7 @@ template _M_initialize_dispatch(_Integer __n, _Integer __x, __true_type) { _M_initialize(static_cast(__n)); - std::fill(this->_M_impl._M_start._M_p, - this->_M_impl._M_end_addr(), __x ? ~0 : 0); + _M_initialize_value(__x); } template @@ -1160,15 +1188,13 @@ template { if (__n > size()) { - std::fill(this->_M_impl._M_start._M_p, - this->_M_impl._M_end_addr(), __x ? ~0 : 0); + _M_initialize_value(__x); insert(end(), __n - size(), __x); } else { _M_erase_at_end(begin() + __n); - std::fill(this->_M_impl._M_start._M_p, - this->_M_impl._M_end_addr(), __x ? ~0 : 0); + _M_initialize_value(__x); } }