Fix std::vector's use of temporary objects
authorJonathan Wakely <jwakely@redhat.com>
Mon, 4 Jul 2016 14:52:46 +0000 (15:52 +0100)
committerJonathan Wakely <redi@gcc.gnu.org>
Mon, 4 Jul 2016 14:52:46 +0000 (15:52 +0100)
* include/bits/stl_vector.h (emplace(const_iterator, _Args&&...)):
Define inline. Forward to _M_emplace_aux.
(insert(const_iterator, value_type&&)): Forward to _M_insert_rval.
(_M_insert_rval, _M_emplace_aux): Declare new functions.
(_Temporary_value): New RAII type using allocator to construct/destroy.
(_S_insert_aux_assign): Remove.
(_M_insert_aux): Make non-variadic.
* include/bits/vector.tcc (insert(const_iterator, const value_type&)):
Use _Temporary_value.
(emplace(const_iterator, _Args&&...)): Remove definition.
(_M_insert_rval, _M_emplace_aux): Define.
(_M_insert_aux): Make non-variadic, stop using _S_insert_aux_assign.
(_M_fill_insert): Use _Temporary_value.
* testsuite/23_containers/vector/allocator/construction.cc: New test.
* testsuite/23_containers/vector/modifiers/insert_vs_emplace.cc:
Adjust expected results for emplacing an lvalue with reallocation.
* testsuite/23_containers/vector/check_construct_destroy.cc: Adjust
expected results to account for construction/destruction of temporary
using allocator.

From-SVN: r237985

libstdc++-v3/ChangeLog
libstdc++-v3/include/bits/stl_vector.h
libstdc++-v3/include/bits/vector.tcc
libstdc++-v3/testsuite/23_containers/vector/allocator/construction.cc [new file with mode: 0644]
libstdc++-v3/testsuite/23_containers/vector/check_construct_destroy.cc
libstdc++-v3/testsuite/23_containers/vector/modifiers/insert_vs_emplace.cc
libstdc++-v3/testsuite/backward/hash_set/check_construct_destroy.cc

index 92d7a4d006c820addebc454882c222825b27703e..d10fc04363f8ba83f61f4d38090078165ce412c6 100644 (file)
@@ -1,3 +1,26 @@
+2016-07-04  Jonathan Wakely  <jwakely@redhat.com>
+
+       * include/bits/stl_vector.h (emplace(const_iterator, _Args&&...)):
+       Define inline. Forward to _M_emplace_aux.
+       (insert(const_iterator, value_type&&)): Forward to _M_insert_rval.
+       (_M_insert_rval, _M_emplace_aux): Declare new functions.
+       (_Temporary_value): New RAII type using allocator to construct/destroy.
+       (_S_insert_aux_assign): Remove.
+       (_M_insert_aux): Make non-variadic.
+       * include/bits/vector.tcc (insert(const_iterator, const value_type&)):
+       Use _Temporary_value.
+       (emplace(const_iterator, _Args&&...)): Remove definition.
+       (_M_insert_rval, _M_emplace_aux): Define.
+       (_M_insert_aux): Make non-variadic, stop using _S_insert_aux_assign.
+       (_M_fill_insert): Use _Temporary_value.
+       * testsuite/23_containers/vector/allocator/construction.cc: New test.
+       * testsuite/23_containers/vector/modifiers/insert_vs_emplace.cc:
+       Adjust expected results for emplacing an lvalue with reallocation.
+       * testsuite/23_containers/vector/check_construct_destroy.cc: Adjust
+       expected results to account for construction/destruction of temporary
+       using allocator.
+       * testsuite/backward/hash_set/check_construct_destroy.cc: Likewise.
+
 2016-07-04  Ville Voutilainen  <ville.voutilainen@gmail.com>
 
        PR libstdc++/71313
index eaafa223dbfd4c99495fe72dd967bddd945de1a0..8e8aa7cf0d1d90e381ee40b5555902924b08d840 100644 (file)
@@ -995,7 +995,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       template<typename... _Args>
        iterator
-       emplace(const_iterator __position, _Args&&... __args);
+       emplace(const_iterator __position, _Args&&... __args)
+       { return _M_emplace_aux(__position, std::forward<_Args>(__args)...); }
 
       /**
        *  @brief  Inserts given value into %vector before specified iterator.
@@ -1040,7 +1041,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       iterator
       insert(const_iterator __position, value_type&& __x)
-      { return emplace(__position, std::move(__x)); }
+      { return _M_insert_rval(__position, std::move(__x)); }
 
       /**
        *  @brief  Inserts an initializer_list into the %vector.
@@ -1431,30 +1432,65 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _M_shrink_to_fit();
 #endif
 
-      // Called by insert(p,x)
 #if __cplusplus < 201103L
+      // Called by insert(p,x)
       void
       _M_insert_aux(iterator __position, const value_type& __x);
 #else
-      template<typename... _Args>
-       static void
-       _S_insert_aux_assign(iterator __pos, _Args&&... __args)
-       { *__pos =  _Tp(std::forward<_Args>(__args)...); }
+      // A value_type object constructed with _Alloc_traits::construct()
+      // and destroyed with _Alloc_traits::destroy().
+      struct _Temporary_value
+      {
+       template<typename... _Args>
+         explicit
+         _Temporary_value(vector* __vec, _Args&&... __args) : _M_this(__vec)
+         {
+           _Alloc_traits::construct(_M_this->_M_impl, _M_ptr(),
+                                    std::forward<_Args>(__args)...);
+         }
 
-      static void
-      _S_insert_aux_assign(iterator __pos, _Tp&& __arg)
-      { *__pos = std::move(__arg); }
+       ~_Temporary_value()
+       { _Alloc_traits::destroy(_M_this->_M_impl, _M_ptr()); }
 
-      template<typename... _Args>
+       value_type&
+       _M_val() { return *reinterpret_cast<_Tp*>(&__buf); }
+
+      private:
+       pointer
+       _M_ptr() { return pointer_traits<pointer>::pointer_to(_M_val()); }
+
+       vector* _M_this;
+       typename aligned_storage<sizeof(_Tp), alignof(_Tp)>::type __buf;
+      };
+
+      // Called by insert(p,x) and other functions when insertion needs to
+      // reallocate or move existing elements. _Arg is either _Tp& or _Tp.
+      template<typename _Arg>
        void
-       _M_insert_aux(iterator __position, _Args&&... __args);
+       _M_insert_aux(iterator __position, _Arg&& __arg);
 
+      // Either move-construct at the end, or forward to _M_insert_aux.
+      iterator
+      _M_insert_rval(const_iterator __position, value_type&& __v);
+
+      // Called by push_back(x) and emplace_back(args) when they need to
+      // reallocate.
       template<typename... _Args>
        void
        _M_emplace_back_aux(_Args&&... __args);
+
+      // Try to emplace at the end, otherwise forward to _M_insert_aux.
+      template<typename... _Args>
+       iterator
+       _M_emplace_aux(const_iterator __position, _Args&&... __args);
+
+      // Emplacing an rvalue of the correct type can use _M_insert_rval.
+      iterator
+      _M_emplace_aux(const_iterator __position, value_type&& __v)
+      { return _M_insert_rval(__position, std::move(__v)); }
 #endif
 
-      // Called by the latter.
+      // Called by _M_fill_insert, _M_insert_aux etc.
       size_type
       _M_check_len(size_type __n, const char* __s) const
       {
index 9cb546403aa97a36cf496259993e8db4fa51ba91..dd0d288eaab82274a6df20751214179f1c5e4fbb 100644 (file)
@@ -124,8 +124,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
          const auto __pos = begin() + (__position - cbegin());
          if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
            {
-             _Tp __x_copy = __x;
-             _M_insert_aux(__pos, std::move(__x_copy));
+             // __x could be an existing element of this vector, so make a
+             // copy of it before _M_insert_aux moves elements around.
+             _Temporary_value __x_copy(this, __x);
+             _M_insert_aux(__pos, std::move(__x_copy._M_val()));
            }
          else
            _M_insert_aux(__pos, __x);
@@ -296,31 +298,50 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       }
 
 #if __cplusplus >= 201103L
+  template<typename _Tp, typename _Alloc>
+    auto
+    vector<_Tp, _Alloc>::
+    _M_insert_rval(const_iterator __position, value_type&& __v) -> iterator
+    {
+      const auto __n = __position - cbegin();
+      if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
+         && __position == cend())
+       {
+         _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
+                                  std::move(__v));
+         ++this->_M_impl._M_finish;
+       }
+      else
+       _M_insert_aux(begin() + __n, std::move(__v));
+      return iterator(this->_M_impl._M_start + __n);
+    }
+
   template<typename _Tp, typename _Alloc>
     template<typename... _Args>
-      typename vector<_Tp, _Alloc>::iterator
+      auto
       vector<_Tp, _Alloc>::
-      emplace(const_iterator __position, _Args&&... __args)
+      _M_emplace_aux(const_iterator __position, _Args&&... __args)
+      -> iterator
       {
-       const size_type __n = __position - begin();
-       if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
-           && __position == end())
+       const auto __n = __position - cbegin();
+       if (__position == cend())
+         emplace_back(std::forward<_Args>(__args)...);
+       else
          {
-           _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
-                                    std::forward<_Args>(__args)...);
-           ++this->_M_impl._M_finish;
+           // We need to construct a temporary because something in __args...
+           // could alias one of the elements of the container and so we
+           // need to use it before _M_insert_aux moves elements around.
+           _Temporary_value __tmp(this, std::forward<_Args>(__args)...);
+           _M_insert_aux(begin() + __n, std::move(__tmp._M_val()));
          }
-       else
-         _M_insert_aux(begin() + (__position - cbegin()),
-                       std::forward<_Args>(__args)...);
        return iterator(this->_M_impl._M_start + __n);
       }
 
   template<typename _Tp, typename _Alloc>
-    template<typename... _Args>
+    template<typename _Arg>
       void
       vector<_Tp, _Alloc>::
-      _M_insert_aux(iterator __position, _Args&&... __args)
+      _M_insert_aux(iterator __position, _Arg&& __arg)
 #else
   template<typename _Tp, typename _Alloc>
     void
@@ -343,7 +364,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 #if __cplusplus < 201103L
          *__position = __x_copy;
 #else
-         _S_insert_aux_assign(__position, std::forward<_Args>(__args)...);
+         *__position = std::forward<_Arg>(__arg);
 #endif
        }
       else
@@ -355,14 +376,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
          pointer __new_finish(__new_start);
          __try
            {
-             // The order of the three operations is dictated by the C++0x
+             // The order of the three operations is dictated by the C++11
              // case, where the moves could alter a new element belonging
              // to the existing vector.  This is an issue only for callers
-             // taking the element by const lvalue ref (see 23.1/13).
+             // taking the element by lvalue ref (see last bullet of C++11
+             // [res.on.arguments]).
              _Alloc_traits::construct(this->_M_impl,
                                       __new_start + __elems_before,
 #if __cplusplus >= 201103L
-                                      std::forward<_Args>(__args)...);
+                                      std::forward<_Arg>(__arg));
 #else
                                       __x);
 #endif
@@ -455,7 +477,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
          if (size_type(this->_M_impl._M_end_of_storage
                        - this->_M_impl._M_finish) >= __n)
            {
+#if __cplusplus < 201103L
              value_type __x_copy = __x;
+#else
+             _Temporary_value __tmp(this, __x);
+             value_type& __x_copy = __tmp._M_val();
+#endif
              const size_type __elems_after = end() - __position;
              pointer __old_finish(this->_M_impl._M_finish);
              if (__elems_after > __n)
diff --git a/libstdc++-v3/testsuite/23_containers/vector/allocator/construction.cc b/libstdc++-v3/testsuite/23_containers/vector/allocator/construction.cc
new file mode 100644 (file)
index 0000000..8040949
--- /dev/null
@@ -0,0 +1,105 @@
+// Copyright (C) 2016 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/>.
+
+// { dg-options "-std=gnu++11" }
+// { dg-do compile }
+
+#include <vector>
+
+struct Tag { };
+
+template<typename T>
+  struct TaggingAllocator
+  {
+    using value_type = T;
+
+    TaggingAllocator() = default;
+
+    template<typename U>
+      TaggingAllocator(const TaggingAllocator<U>&) { }
+
+    T*
+    allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
+
+    void
+    deallocate(T* p, std::size_t n) { std::allocator<T>{}.deallocate(p, n); }
+
+    template<typename U, typename... Args>
+      void
+      construct(U* p, Args&&... args)
+      { ::new((void*)p) U(Tag{}, std::forward<Args>(args)...); }
+
+    template<typename U, typename... Args>
+      void
+      destroy(U* p)
+      { p->~U(); }
+  };
+
+template<typename T, typename U>
+  bool
+  operator==(const TaggingAllocator<T>&, const TaggingAllocator<U>&)
+  { return true; }
+
+template<typename T, typename U>
+  bool
+  operator!=(const TaggingAllocator<T>&, const TaggingAllocator<U>&)
+  { return false; }
+
+struct X
+{
+  // All constructors must be passed the Tag type.
+
+  // DefaultInsertable into vector<X, TaggingAllocator<X>>,
+  X(Tag) { }
+  // CopyInsertable into vector<X, TaggingAllocator<X>>,
+  X(Tag, const X&) { }
+  // MoveInsertable into vector<X, TaggingAllocator<X>>, and
+  X(Tag, X&&) { }
+
+  // EmplaceConstructible into vector<X, TaggingAllocator<X>> from args.
+  template<typename... Args>
+    X(Tag, Args&&...) { }
+
+  // not DefaultConstructible, CopyConstructible or MoveConstructible.
+  X() = delete;
+  X(const X&) = delete;
+  X(X&&) = delete;
+
+  // CopyAssignable.
+  X& operator=(const X&) { return *this; }
+
+  // MoveAssignable.
+  X& operator=(X&&) { return *this; }
+
+private:
+  // Not Destructible.
+  ~X() { }
+
+  // Erasable from vector<X, TaggingAllocator<X>>.
+  friend class TaggingAllocator<X>;
+};
+
+template class std::vector<X, TaggingAllocator<X>>;
+
+void test01()
+{
+  std::vector<X, TaggingAllocator<X>> v;
+  v.reserve(3);
+  v.emplace_back();
+  v.emplace(v.begin());
+  v.emplace(v.begin(), 1, 2, 3);
+}
index cf2f7c7a0d7c5afd2c75a21fa92a1322d82ff43f..b92a1521aa258861679ca5c36258769582395625 100644 (file)
@@ -49,9 +49,9 @@ int main()
     c.reserve(100);
     tracker_allocator_counter::reset();
     c.insert(c.begin(), arr10[0]);
-    ok = check_construct_destroy("Insert element", 1, 0) && ok;
+    ok = check_construct_destroy("Insert element", 2, 1) && ok;
   }
-  ok = check_construct_destroy("Insert element", 1, 11) && ok;
+  ok = check_construct_destroy("Insert element", 2, 12) && ok;
 
   {
     Container c(arr10, arr10 + 10);
index 39a3f031b461a705eba5020b1ed5d9b7ebe6e5de..1b461240592c77fc0b3fef71e828abf2cfc53176 100644 (file)
@@ -223,7 +223,8 @@ test03()
 void
 test04()
 {
-  const X::special expected{ 0, 3, 1, 0, 3, 0 };
+  const X::special expected_ins{ 0, 3, 1, 0, 3, 0 };
+  const X::special expected_emp{ 0, 4, 1, 0, 4, 0 };
   X::special ins, emp;
   {
     std::vector<X> v;
@@ -253,8 +254,8 @@ test04()
     // std::cout << "----\n";
     emp = X::sp;
   }
-  VERIFY( ins == emp );
-  VERIFY( ins == expected );
+  VERIFY( ins == expected_ins );
+  VERIFY( emp == expected_emp );
 }
 
 // insert vs emplace xvalue reallocation
index 5693c76c3863b038dfa44b699ae54566686970fb..5740fe1bf8b15c54479c1b8a3da119775668dcff 100644 (file)
@@ -39,45 +39,48 @@ int main()
 
   int buckets;
 
+  // Add 1 to all counts, because the std::vector used internally by the
+  // hashtable creates and destroys a temporary object using the allocator.
+
   tracker_allocator_counter::reset();
   {
     Container c;
     buckets = c.bucket_count();
-    ok = check_construct_destroy("empty container", buckets, 0) && ok;
+    ok = check_construct_destroy("empty container", buckets+1, 1) && ok;
   }
-  ok = check_construct_destroy("empty container", buckets, buckets) && ok;
+  ok = check_construct_destroy("empty container", buckets+1, buckets+1) && ok;
 
 
   tracker_allocator_counter::reset();
   {
     Container c(arr10, arr10 + 10);
-    ok = check_construct_destroy("Construct from range", buckets+10, 0) && ok;
+    ok = check_construct_destroy("Construct from range", buckets+10+1, 1) && ok;
   }
-  ok = check_construct_destroy("Construct from range", buckets+10, buckets+10) && ok;
+  ok = check_construct_destroy("Construct from range", buckets+10+1, buckets+10+1) && ok;
 
   tracker_allocator_counter::reset();
   {
     Container c(arr10, arr10 + 10);
     c.insert(arr10a[0]);
-    ok = check_construct_destroy("Insert element", buckets+11, 0) && ok;
+    ok = check_construct_destroy("Insert element", buckets+11+1, 1) && ok;
   }
-  ok = check_construct_destroy("Insert element", buckets+11, buckets+11) && ok;
+  ok = check_construct_destroy("Insert element", buckets+11+1, buckets+11+1) && ok;
 
   tracker_allocator_counter::reset();
   {
     Container c(arr10, arr10 + 10);
     c.insert(arr10a, arr10a+3);
-    ok = check_construct_destroy("Insert short range", buckets+13, 0) && ok;
+    ok = check_construct_destroy("Insert short range", buckets+13+1, 1) && ok;
   }
-  ok = check_construct_destroy("Insert short range", buckets+13, buckets+13) && ok;
+  ok = check_construct_destroy("Insert short range", buckets+13+1, buckets+13+1) && ok;
 
   tracker_allocator_counter::reset();
   {
     Container c(arr10, arr10 + 10);
     c.insert(arr10a, arr10a+10);
-    ok = check_construct_destroy("Insert long range", buckets+20, 0) && ok;
+    ok = check_construct_destroy("Insert long range", buckets+20+1, 1) && ok;
   }
-  ok = check_construct_destroy("Insert long range", buckets+20, buckets+20) && ok;
+  ok = check_construct_destroy("Insert long range", buckets+20+1, buckets+20+1) && ok;
 
   return ok ? 0 : 1;
 }