sized delete in _Temporary_buffer<>

Message ID b85d0bf5-7c8a-050a-1d5a-1f716edc2e1d@gmail.com
State New
Headers show
Series
  • sized delete in _Temporary_buffer<>
Related show

Commit Message

François Dumont July 18, 2019, 5:41 a.m.
As we adopted the sized deallocation in the new_allocator why not doing 
the same in _Temporary_buffer<>.

     * include/bits/stl_tempbuf.h (__detail::__return_temporary_buffer): 
New.
     (~_Temporary_buffer()): Use latter.
     (_Temporary_buffer(_FIterator, size_type)): Likewise.

Tested w/o activating sized deallocation. I'll try to run tests with 
this option activated.

Ok to commit ?

François

Comments

Jonathan Wakely July 18, 2019, 10:09 a.m. | #1
On 18/07/19 07:41 +0200, François Dumont wrote:
>As we adopted the sized deallocation in the new_allocator why not 

>doing the same in _Temporary_buffer<>.

>

>    * include/bits/stl_tempbuf.h 

>(__detail::__return_temporary_buffer): New.

>    (~_Temporary_buffer()): Use latter.

>    (_Temporary_buffer(_FIterator, size_type)): Likewise.

>

>Tested w/o activating sized deallocation. I'll try to run tests with 

>this option activated.


As the manual says, it's enabled by default for C++14 and later.

>Ok to commit ?


OK for trunk, thanks.
Morwenn Ed July 19, 2019, 7:46 p.m. | #2
If I'm not mistaken this patch allocates N*sizeof(_Tp) bytes of storage and deallocates N bytes when sized deallocation is enabled?

Shouldn't __return_temporary_buffer deallocate N*sizeof(_Tp) instead to match the value passed to new?

________________________________
De : libstdc++-owner@gcc.gnu.org <libstdc++-owner@gcc.gnu.org> de la part de François Dumont <frs.dumont@gmail.com>
Envoyé : jeudi 18 juillet 2019 07:41
À : libstdc++@gcc.gnu.org <libstdc++@gcc.gnu.org>; gcc-patches <gcc-patches@gcc.gnu.org>
Objet : sized delete in _Temporary_buffer<>

As we adopted the sized deallocation in the new_allocator why not doing
the same in _Temporary_buffer<>.

     * include/bits/stl_tempbuf.h (__detail::__return_temporary_buffer):
New.
     (~_Temporary_buffer()): Use latter.
     (_Temporary_buffer(_FIterator, size_type)): Likewise.

Tested w/o activating sized deallocation. I'll try to run tests with
this option activated.

Ok to commit ?

François
François Dumont July 19, 2019, 9:23 p.m. | #3
(2nd sent attempt as text this time.)

Good spot, fixed with attached patch, committed as trivial.

2019-07-19  François Dumont <fdumont@gcc.gnu.org>

     * include/bits/stl_tempbuf.h (__detail::__return_temporary_buffer): Fix
     sized deallocation size computation.


On 7/19/19 9:46 PM, Morwenn Ed wrote:
> If I'm not mistaken this patch allocates N*sizeof(_Tp) bytes of 

> storage and deallocates N bytes when sized deallocation is enabled?

>

> Shouldn't __return_temporary_buffer deallocate N*sizeof(_Tp) instead 

> to match the value passed to new?

>

> ------------------------------------------------------------------------

> *De :* libstdc++-owner@gcc.gnu.org <libstdc++-owner@gcc.gnu.org> de la 

> part de François Dumont <frs.dumont@gmail.com>

> *Envoyé :* jeudi 18 juillet 2019 07:41

> *À :* libstdc++@gcc.gnu.org <libstdc++@gcc.gnu.org>; gcc-patches 

> <gcc-patches@gcc.gnu.org>

> *Objet :* sized delete in _Temporary_buffer<>

> As we adopted the sized deallocation in the new_allocator why not doing

> the same in _Temporary_buffer<>.

>

>      * include/bits/stl_tempbuf.h (__detail::__return_temporary_buffer):

> New.

>      (~_Temporary_buffer()): Use latter.

>      (_Temporary_buffer(_FIterator, size_type)): Likewise.

>

> Tested w/o activating sized deallocation. I'll try to run tests with

> this option activated.

>

> Ok to commit ?

>

> François

>
diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h b/libstdc++-v3/include/bits/stl_tempbuf.h
index bb7c2cd1334..ce3f3624437 100644
--- a/libstdc++-v3/include/bits/stl_tempbuf.h
+++ b/libstdc++-v3/include/bits/stl_tempbuf.h
@@ -71,7 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 				size_t __len __attribute__((__unused__)))
       {
 #if __cpp_sized_deallocation
-	::operator delete(__p, __len);
+	::operator delete(__p, __len * sizeof(_Tp));
 #else
 	::operator delete(__p);
 #endif

Patch

diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h b/libstdc++-v3/include/bits/stl_tempbuf.h
index b6ad9ee6a46..bb7c2cd1334 100644
--- a/libstdc++-v3/include/bits/stl_tempbuf.h
+++ b/libstdc++-v3/include/bits/stl_tempbuf.h
@@ -63,6 +63,21 @@  namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
+  namespace __detail
+  {
+    template<typename _Tp>
+      inline void
+      __return_temporary_buffer(_Tp* __p,
+				size_t __len __attribute__((__unused__)))
+      {
+#if __cpp_sized_deallocation
+	::operator delete(__p, __len);
+#else
+	::operator delete(__p);
+#endif
+      }
+  }
+
   /**
    *  @brief Allocates a temporary buffer.
    *  @param  __len  The number of objects of type Tp.
@@ -112,7 +127,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return_temporary_buffer(_Tp* __p)
     { ::operator delete(__p); }
 
-
   /**
    *  This class is used in two places: stl_algo.h and ext/memory,
    *  where it is wrapped as the temporary_buffer class.  See
@@ -165,7 +179,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       ~_Temporary_buffer()
       {
 	std::_Destroy(_M_buffer, _M_buffer + _M_len);
-	std::return_temporary_buffer(_M_buffer);
+	std::__detail::__return_temporary_buffer(_M_buffer, _M_len);
       }
 
     private:
@@ -185,7 +199,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         __ucr(_Pointer __first, _Pointer __last,
 	      _ForwardIterator __seed)
         {
-	  if(__first == __last)
+	  if (__first == __last)
 	    return;
 
 	  _Pointer __cur = __first;
@@ -244,22 +258,23 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _Temporary_buffer(_ForwardIterator __seed, size_type __original_len)
     : _M_original_len(__original_len), _M_len(0), _M_buffer(0)
     {
-      __try
-	{
-	  std::pair<pointer, size_type> __p(std::get_temporary_buffer<
-					    value_type>(_M_original_len));
-	  _M_buffer = __p.first;
-	  _M_len = __p.second;
-	  if (_M_buffer)
-	    std::__uninitialized_construct_buf(_M_buffer, _M_buffer + _M_len,
-					       __seed);
-	}
-      __catch(...)
+      std::pair<pointer, size_type> __p(
+		std::get_temporary_buffer<value_type>(_M_original_len));
+
+      if (__p.first)
 	{
-	  std::return_temporary_buffer(_M_buffer);
-	  _M_buffer = 0;
-	  _M_len = 0;
-	  __throw_exception_again;
+	  __try
+	    {
+	      std::__uninitialized_construct_buf(__p.first, __p.first + __p.second,
+						 __seed);
+	      _M_buffer = __p.first;
+	      _M_len = __p.second;
+	    }
+	  __catch(...)
+	    {
+	      std::__detail::__return_temporary_buffer(__p.first, __p.second);
+	      __throw_exception_again;
+	    }
 	}
     }