From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110290 invoked by alias); 18 Jul 2017 13:28:41 -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 110263 invoked by uid 89); 18 Jul 2017 13:28:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=UD:B, Hx-languages-length:4637 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; Tue, 18 Jul 2017 13:28:38 +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 647954A6E3; Tue, 18 Jul 2017 13:28:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 647954A6E3 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jwakely@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 647954A6E3 Received: from localhost (unknown [10.33.36.81]) by smtp.corp.redhat.com (Postfix) with ESMTP id E063C6F948; Tue, 18 Jul 2017 13:28:36 +0000 (UTC) Date: Tue, 18 Jul 2017 13:28:00 -0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: Default std::list default and move constructors Message-ID: <20170718132836.GA15340@redhat.com> References: <20170705152254.GD15340@redhat.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: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.8.0 (2017-02-23) X-SW-Source: 2017-07/txt/msg01065.txt.bz2 On 12/07/17 22:12 +0200, François Dumont wrote: >On 05/07/2017 17:22, Jonathan Wakely wrote: >>It's mostly good, but I'd like to make a few suggestions ... >> >> >>>diff --git a/libstdc++-v3/include/bits/stl_list.h >>>b/libstdc++-v3/include/bits/stl_list.h >>>index 232885a..7ffffe5 100644 >>>--- a/libstdc++-v3/include/bits/stl_list.h >>>+++ b/libstdc++-v3/include/bits/stl_list.h >>>@@ -82,6 +82,17 @@ namespace std _GLIBCXX_VISIBILITY(default) >>> _List_node_base* _M_next; >>> _List_node_base* _M_prev; >>> >>>+#if __cplusplus >= 201103L >>>+ _List_node_base() = default; >>>+#else >>>+ _List_node_base() >>>+ { } >>>+#endif >>>+ >>>+ _List_node_base(_List_node_base* __next, _List_node_base* __prev) >>>+ : _M_next(__next), _M_prev(__prev) >>>+ { } >>>+ >> >>I think I'd prefer to leave this struct with no user-defined >>constructors, instead of adding these. > >My goal was to make sure that as much as possible instances are >initialized through their initialization list. But it is still >possible without it so I made the change. > >> >>> static void >>> swap(_List_node_base& __x, _List_node_base& __y) >>>_GLIBCXX_USE_NOEXCEPT; >>> >>>@@ -99,6 +110,79 @@ namespace std _GLIBCXX_VISIBILITY(default) >>> _M_unhook() _GLIBCXX_USE_NOEXCEPT; >>> }; >>> >>>+ /// The %list node header. >>>+ struct _List_node_header : public _List_node_base >>>+ { >>>+ private: >>>+#if _GLIBCXX_USE_CXX11_ABI >>>+ std::size_t _M_size; >>>+#endif >> >>I don't think this needs to be private, because we don't have to worry >>about users accessing this member. It's an internal-only type, and the >>_M_next and _M_prev members are already public. > >It's not internal-only as those members are exposed on std::list >through inheritance. But I agree that consistency is more important >here so I made it public. Looks like it's only used as a base class of _List_impl, which is private to std::list, and in the __distance overload. Am I missing something? >> >>If it's public then the _List_base::_M_inc_size, _M_dec_size etc. >>could access it directly, and we don't need to add duplicates of those >>functions to _List_impl. >> >>>+ >>>+ _List_node_base* _M_base() { return this; } >> >>Is this function necessary? > >It is a nice replacement for calls to __addressof. OK, that does make it more readable. >>>-#if _GLIBCXX_USE_CXX11_ABI >>>- size_t _M_get_size() const { return >>>*_M_impl._M_node._M_valptr(); } >>>+ size_t >>>+ _M_get_size() const { return _M_impl._M_node._M_get_size(); } >>> >>>- void _M_set_size(size_t __n) { *_M_impl._M_node._M_valptr() >>>= __n; } >>>+ void >>>+ _M_set_size(size_t __n) { _M_impl._M_node._M_set_size(__n); } >>> >>>- void _M_inc_size(size_t __n) { *_M_impl._M_node._M_valptr() >>>+= __n; } >>>+ void >>>+ _M_inc_size(size_t __n) { _M_impl._M_node._M_inc_size(__n); } >>> >>>- void _M_dec_size(size_t __n) { *_M_impl._M_node._M_valptr() >>>-= __n; } >>>+ void >>>+ _M_dec_size(size_t __n) { _M_impl._M_node._M_dec_size(__n); } >> >>These functions could just access _M_impl._M_size directly if it was >>public. Introducing new functions to _List_impl to be used here seems >>like unnecessary complication. We don't get rid of the #if because >>it's still needed for these functions anyway: >> >Yes, I wanted to manage as much as possible usage of C++11 abi in the >new _List_node_header type. N.B. the ABI is called "cxx11" not C++11. It's a bad name, I should have called it something else, but please don't make it worse by saying "C++11". That ABI is also the default for C++98 so saying "C++11" just causes more confusion than necessary. You're not getting rid of the #if entirely, so some of the cxx11 ABI specialization already lives in _List_base. Adding several new member functions and still failing to remove the #if#else from _List_base didn't seem like an improvement. The new patch looks much better to me. >So here is the new patch limited to this evolution. Optimization for >always equal allocator will come after along with another >simplification to replace _S_distance with std::distance. OK. >Tests running, ok to commit if successful ? OK for trunk with one tiny tweak, while you're already changing the function ... >@@ -1983,12 +2011,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _GLIBCXX_STD_C::_List_const_iterator<_Tp> __last, > input_iterator_tag) > { >- typedef _GLIBCXX_STD_C::_List_node _Sentinel; >+ typedef __detail::_List_node_header _Sentinel; > _GLIBCXX_STD_C::_List_const_iterator<_Tp> __beyond = __last; > ++__beyond; > bool __whole = __first == __beyond; Could you please make __whole const? Thanks!