Patchwork [v3] libstdc++/55979 (+ notes about 55977)

login
register
mail settings
Submitter Paolo Carlini
Date March 17, 2013, 5:14 p.m.
Message ID <5145F9FC.7090808@oracle.com>
Download mbox | patch
Permalink /patch/228299/
State New
Headers show

Comments

Paolo Carlini - March 17, 2013, 5:14 p.m.
Hi,

I had a look to these two PRs and, as regards the former, the below 
patchlet seems Ok to me even for 4.8.1.

About the latter: a similar change works fine for std::vector and 
std::deque, but std::list for example has another problem, which is 
already exposed by the v1.emplace_back(i); bit and seems unrelated: we have:

   template<typename _Tp>
     struct _List_node : public __detail::_List_node_base
     {
       ///< User's data.
       _Tp _M_data;

#if __cplusplus >= 201103L
       template<typename... _Args>
         _List_node(_Args&&... __args)
     : __detail::_List_node_base(), _M_data(std::forward<_Args>(__args)...)
         { }
#endif
     };

together with:

       template<typename... _Args>
         _Node*
         _M_create_node(_Args&&... __args)
     {
       _Node* __p = this->_M_get_node();
       __try
         {
           _M_get_Node_allocator().construct(__p,
                         std::forward<_Args>(__args)...);
         }
       __catch(...)
         {
           _M_put_node(__p);
           __throw_exception_again;
         }
       return __p;
     }

and this is not Ok in terms of access control, because _List_node tries 
to use the constructor, not the allocator (not sure how strict the 
Standard is in terms of access control)

I guess we could at least work around the problem by going back to 
_M_get_Tp_allocator().construct in _M_create_node (or, better, the 
allocator_traits<>::construct equivalent, per the recent fix for 56613; 
we would use it on _Tp actually, everywhere) but I don't know if Jon has 
already something in his tree for this batch of issues regarding our 
base container class / node constructors, or we want to decouple the 
issue from 55977, do std::vector and std::deque, which would be trivial 
even for 4.8.1, or something else. Suggestions?

Thanks!
Paolo.

//////////////////////////
2013-03-17  Paolo Carlini  <paolo.carlini@oracle.com>

	PR libstdc++/55979
	* include/bits/stl_list.h (_M_initialize_dispatch(_InputIterator,
	_InputIterator, __false_type)): Use emplace_back.
	* testsuite/23_containers/list/cons/55979.cc: New.
	* testsuite/23_containers/list/modifiers/1.h: Adjust.
	* testsuite/23_containers/list/requirements/dr438/assign_neg.cc:
	Adjust dg-error line number.
Jonathan Wakely - March 17, 2013, 5:45 p.m.
On 17 March 2013 17:14, Paolo Carlini wrote:
>
> I guess we could at least work around the problem by going back to
> _M_get_Tp_allocator().construct in _M_create_node (or, better, the
> allocator_traits<>::construct equivalent, per the recent fix for 56613; we
> would use it on _Tp actually, everywhere) but I don't know if Jon has
> already something in his tree for this batch of issues regarding our base
> container class / node constructors, or we want to decouple the issue from
> 55977, do std::vector and std::deque, which would be trivial even for 4.8.1,
> or something else. Suggestions?

For std::list I'm waiting until we have two separate C++03 and C++11
implementations, then I'll implement allocator support in the C++11
code only, as it will be much easier.

To be correct the List_node::_M_data member needs to be replaced with
aligned_storage<sizeof(_Tp)>::type, so the _List_node gets constructed
with uninitialized memory, then use allocator_traits::construct() to
initialize the buffer.

(Because that pattern occurs in several places now I'm going to
introduce an aligned_buffer<_Tp> template that simplifies it.)

Patch

Index: include/bits/stl_list.h
===================================================================
--- include/bits/stl_list.h	(revision 196754)
+++ include/bits/stl_list.h	(working copy)
@@ -1487,7 +1487,11 @@ 
 			       __false_type)
         {
 	  for (; __first != __last; ++__first)
+#if __cplusplus >= 201103L
+	    emplace_back(*__first);
+#else
 	    push_back(*__first);
+#endif
 	}
 
       // Called by list(n,v,a), and the range constructor when it turns out
Index: testsuite/23_containers/list/cons/55979.cc
===================================================================
--- testsuite/23_containers/list/cons/55979.cc	(revision 0)
+++ testsuite/23_containers/list/cons/55979.cc	(working copy)
@@ -0,0 +1,34 @@ 
+// { dg-do compile }
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2013 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <list>
+
+struct A
+{
+  A(int) { }
+  A(const A&) = delete;
+  A& operator=(const A&) = delete;
+};
+
+void foo()
+{
+  int i[] = {1, 2};
+  std::list<A> li(i, i + 2);
+}
Index: testsuite/23_containers/list/modifiers/1.h
===================================================================
--- testsuite/23_containers/list/modifiers/1.h	(revision 196748)
+++ testsuite/23_containers/list/modifiers/1.h	(working copy)
@@ -89,14 +89,22 @@ 
   b = list0301.begin();
   list0301.insert(b, A, A + N); // should be [321 322 333 13 13]
   VERIFY(list0301.size() == 5);
+#if __cplusplus >= 201103L
+  VERIFY(copy_constructor::count() == 0);
+#else
   VERIFY(copy_constructor::count() == 3);
+#endif
   VERIFY(m->id() == 13);
 
   // range fill at end
   value_type::reset();
   list0301.insert(e, A, A + N); // should be [321 322 333 13 13 321 322 333]
   VERIFY(list0301.size() == 8);
+#if __cplusplus >= 201103L
+  VERIFY(copy_constructor::count() == 0);
+#else
   VERIFY(copy_constructor::count() == 3);
+#endif
   VERIFY(e == list0301.end());
   VERIFY(m->id() == 13);
 
@@ -104,7 +112,11 @@ 
   value_type::reset();
   list0301.insert(m, A, A + N);
   VERIFY(list0301.size() == 11);
+#if __cplusplus >= 201103L
+  VERIFY(copy_constructor::count() == 0);
+#else
   VERIFY(copy_constructor::count() == 3);
+#endif
   VERIFY(e == list0301.end());
   VERIFY(m->id() == 13);
 
Index: testsuite/23_containers/list/requirements/dr438/assign_neg.cc
===================================================================
--- testsuite/23_containers/list/requirements/dr438/assign_neg.cc	(revision 196748)
+++ testsuite/23_containers/list/requirements/dr438/assign_neg.cc	(working copy)
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1525 }
+// { dg-error "no matching" "" { target *-*-* } 1529 }
 
 #include <list>