From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26939 invoked by alias); 15 May 2014 20:52:30 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26915 invoked by uid 89); 15 May 2014 20:52:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Thu, 15 May 2014 20:52:27 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4FKqPf0024009 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 15 May 2014 16:52:25 -0400 Received: from localhost (vpn1-7-12.ams2.redhat.com [10.36.7.12]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4FKqMLw029145; Thu, 15 May 2014 16:52:24 -0400 Date: Thu, 15 May 2014 20:52:00 -0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [patch] libstdc++/61143 make unordered containers usable after move Message-ID: <20140515205221.GA30753@redhat.com> References: <53752198.1030705@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <53752198.1030705@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2014-05/txt/msg01229.txt.bz2 On 15/05/14 22:20 +0200, François Dumont wrote: >Hi > > Here is a proposal to fix PR 61143. As explained in the PR I >finally prefer to leave the container in a valid state that is to say >with a non zero number of bucket, that is to say 1, after a move. This >bucket is stored directly in the container so not allocated to leave >the move operations noexcept qualified. Thanks for fixing this, I like the direction very much. I have a few comments below ... > With this evolution we could >even make the default constructor noexcept but I don't think it has >any interest. I tend to agree with Paolo that a noexcept default constructor might be useful, but let's fix the bug first and consider that later. >Index: include/bits/hashtable.h >=================================================================== >--- include/bits/hashtable.h (revision 210479) >+++ include/bits/hashtable.h (working copy) >@@ -316,14 +316,37 @@ > size_type _M_element_count; > _RehashPolicy _M_rehash_policy; > >+ // A single bucket used when only need 1 bucket. After move the hashtable >+ // is left with only 1 bucket which is not allocated so that we can have a >+ // noexcept move constructor. >+ // Note that we can't leave hashtable with 0 bucket without adding >+ // numerous checks in the code to avoid 0 modulus. >+ __bucket_type _M_single_bucket; Does this get initialized in the constructors? Would it make sense to give it an initializer? __bucket_type _M_single_bucket = nullptr; >@@ -980,12 +999,16 @@ > _M_move_assign(_Hashtable&& __ht, std::true_type) > { > this->_M_deallocate_nodes(_M_begin()); >- if (__builtin_expect(_M_bucket_count != 0, true)) >- _M_deallocate_buckets(); >- >+ _M_deallocate_buckets(); > __hashtable_base::operator=(std::move(__ht)); > _M_rehash_policy = __ht._M_rehash_policy; >- _M_buckets = __ht._M_buckets; >+ if (__builtin_expect(__ht._M_buckets != &__ht._M_single_bucket, true)) >+ _M_buckets = __ht._M_buckets; What is the value of this->_M_single_bucket now? Should it be set to nullptr, if only to help debugging? >+ if (__builtin_expect(__ht._M_buckets == &__ht._M_single_bucket, false)) This check appears in a few places, I wonder if it is worth creating a private member function to hide the details: bool _M_moved_from() const noexcept { return __builtin_expect(_M_buckets == &_M_single_bucket, false); } Then just test if (__ht._M_moved_from()) Usually I would think the __builtin_expect should not be inside the member function, so the caller decides what the expected result is, but I think in all cases the result is expected to be false. That matches how move semantics are designed: the object that gets moved from is expected to be going out of scope, and so will be reused in a minority of cases. >@@ -1139,7 +1170,14 @@ > { > if (__ht._M_node_allocator() == this->_M_node_allocator()) > { >- _M_buckets = __ht._M_buckets; >+ if (__builtin_expect(__ht._M_buckets == &__ht._M_single_bucket, false)) This could be if (__ht._M_moved_from()) >@@ -1193,11 +1231,21 @@ > std::swap(_M_bucket_count, __x._M_bucket_count); > std::swap(_M_before_begin._M_nxt, __x._M_before_begin._M_nxt); > std::swap(_M_element_count, __x._M_element_count); >+ std::swap(_M_single_bucket, __x._M_single_bucket); > >+ // Fix buckets if any is pointing to its single bucket that can't be >+ // swapped. >+ if (_M_buckets == &__x._M_single_bucket) >+ _M_buckets = &_M_single_bucket; >+ >+ if (__x._M_buckets == &_M_single_bucket) >+ __x._M_buckets = &__x._M_single_bucket; >+ Does this do more work than necessary, swapping the _M_buckets members, then updating them again? How about removing the std::swap(_M_buckets, __x._M_buckets) above and doing (untested): if (this->_M_moved_from()) { if (__x._M_moved_from()) _M_buckets = &_M_single_bucket; else _M_buckets = __x._M_buckets; __x._M_buckets = &__x._M_single_bucket; } else if (__x._M_moved_from()) { __x._M_buckets = _M_buckets; _M_buckets = &_M_single_bucket; } else std::swap(_M_buckets, __x._M_buckets); Is that worth it? I'm not sure. >Index: testsuite/23_containers/unordered_map/allocator/copy_assign.cc >=================================================================== >--- testsuite/23_containers/unordered_map/allocator/copy_assign.cc (revision 210479) >+++ testsuite/23_containers/unordered_map/allocator/copy_assign.cc (working copy) The changes to this file are not listed in the ChangeLog entry, and we don't want writes to stdout in the testsuite. Did you intend to include this in the patch? >Index: testsuite/Makefile.in >=================================================================== >--- testsuite/Makefile.in (revision 210479) >+++ testsuite/Makefile.in (working copy) Same question for this file.