On 12/05/19 21:22 +0000, Andrew Luo wrote:
It's been a while, but thought I'd check in again now that GCC 9 has been
branched, and master is now on GCC 10.
I've merged my changes with the latest code and attached a diff... Let me know
if there's any thoughts on this.
Well I failed to get around to this for GCC 10, so let's try again
now.
I've done another review of the patch, and I'm now wondering why the
new _M_shrink function takes a requested capacity, when the caller
always passes zero. We can simplify both implementations of _M_shrink
if we remove the parameter and change the callers from _M_shrink(0) to
_M_shrink().
Was there a reason to make it take an argument? Did you anticipate
future uses of _M_shrink(n) for n >= 0?
If we simplify it then we need fewer branches in _M_shrink, because we
don't need to do:
// Make sure we don't shrink below the current size.
if (__requested_capacity < length())
__requested_capacity = length();
We only need to check whether length() < capacity() (and whether the
string is shared, for the COW implementation).
And if we do that, we can get rid of _M_shrink() because it's now
identical to the zero-argument form of reserve(). So we can just put
the body of _M_shrink() straight in reserve(). The reserve() function
is deprecated, but when we need to remove it we can just make it
private, so that shrink_to_fit() can still call it.
The only downside of this I see is that when the deprecated reserve()
eventually gets removed from the standard, our users will get a
"reserve() is private" error rather than a "wrong number of arguments"
error. But that might actually be better, since they can go to the
location of the private member and see the comments and attribute
describing its status in different standard versions.
I've attached a relative diff showing my suggested changes to your
most recent patch. This also fixes some regressions, because the
_M_shrink function was not swallowing exceptions that result from a
failure to reallocate, which shrink_to_fit() was doing previously.
What do you think?
commit 518cfa3c6e344e0346c51c7cab53a71309d0f730
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Thu Jul 30 15:09:55 2020
Simplify
libstdc++-v3/ChangeLog:
* config/abi/pre/gnu.ver: Change _M_shrink exports to reserve().
* include/bits/basic_string.h: Simplify.
* include/bits/basic_string.tcc: Simplify.
diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 2a88951fa52..3ff7bb61002 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2310,10 +2310,10 @@ GLIBCXX_3.4.29 {
# std::from_chars
_ZSt10from_charsPKcS0_R[def]St12chars_format;
- # basic_string::_M_shrink
- _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE9_M_shrinkEm;
- _ZNSs9_M_shrinkEm;
- _ZNSbIwSt11char_traitsIwESaIwEE9_M_shrinkEm;
+ # basic_string::reserve()
+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserveEv;
+ _ZNSs7reserveEv;
+ _ZNSbIwSt11char_traitsIwESaIwEE7reserveEv;
} GLIBCXX_3.4.28;
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 9ee7a7d3ac4..1c07aa0aa07 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -420,9 +420,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
void
_M_erase(size_type __pos, size_type __n);
- void
- _M_shrink(size_type __requested_capacity);
-
public:
// Construct/copy/destroy:
// NB: We overload ctors in some cases instead of using default
@@ -943,10 +940,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
{ this->resize(__n, _CharT()); }
#if __cplusplus >= 201103L
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
/// A non-binding request to reduce capacity() to size().
void
shrink_to_fit() noexcept
- { _M_shrink(0); }
+ { reserve(); }
+#pragma GCC diagnostic pop
#endif
/**
@@ -983,13 +983,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
/**
* Equivalent to shrink_to_fit().
*/
- __attribute__((__always_inline__))
#if __cplusplus > 201703L
[[deprecated("use shrink_to_fit() instead")]]
#endif
void
- reserve()
- { _M_shrink(0); }
+ reserve();
/**
* Erases the string, making it empty.
@@ -3946,10 +3944,13 @@ _GLIBCXX_END_NAMESPACE_CXX11
{ this->resize(__n, _CharT()); }
#if __cplusplus >= 201103L
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
/// A non-binding request to reduce capacity() to size().
void
- shrink_to_fit() _GLIBCXX_NOEXCEPT
- { _M_shrink(0); }
+ shrink_to_fit() noexcept
+ { reserve(); }
+#pragma GCC diagnostic pop
#endif
/**
@@ -3980,16 +3981,12 @@ _GLIBCXX_END_NAMESPACE_CXX11
void
reserve(size_type __res_arg);
- /**
- * Equivalent to shrink_to_fit().
- */
- __attribute__((__always_inline__))
+ /// Equivalent to shrink_to_fit().
#if __cplusplus > 201703L
[[deprecated("use shrink_to_fit() instead")]]
#endif
void
- reserve()
- { _M_shrink(0); }
+ reserve();
/**
* Erases the string, making it empty.
@@ -5115,9 +5112,6 @@ _GLIBCXX_END_NAMESPACE_CXX11
_M_replace_safe(size_type __pos1, size_type __n1, const _CharT* __s,
size_type __n2);
- void
- _M_shrink(size_type __requested_capacity);
-
// _S_construct_aux is used to implement the 21.3.1 para 15 which
// requires special behaviour if _InIter is an integral type
template<class _InIterator>
diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
index 614beab6864..485b01d12b8 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -285,7 +285,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// 2968. Inconsistencies between basic_string reserve and
// vector/unordered_map/unordered_set reserve functions
// P0966 reserve should not shrink
- if (__res <= capacity())
+ if (__res <= __capacity)
return;
pointer __tmp = _M_create(__res, __capacity);
@@ -335,35 +335,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template<typename _CharT, typename _Traits, typename _Alloc>
void
basic_string<_CharT, _Traits, _Alloc>::
- _M_shrink(size_type __requested_capacity)
+ reserve()
{
if (_M_is_local())
return;
- // Make sure we don't shrink below the current size.
- if (__requested_capacity < length())
- __requested_capacity = length();
+ const size_type __length = length();
+ const size_type __capacity = _M_allocated_capacity;
- const size_type __capacity = capacity();
- if (__requested_capacity < __capacity)
+ if (__length <= size_type(_S_local_capacity))
{
- if (__requested_capacity <= size_type(_S_local_capacity))
- {
- this->_S_copy(_M_local_data(), _M_data(), length() + 1);
- _M_destroy(__capacity);
- _M_data(_M_local_data());
- }
-#if __cpp_exceptions
- else
- {
- pointer __tmp = _M_create(__requested_capacity, __capacity);
- this->_S_copy(__tmp, _M_data(), length() + 1);
- _M_dispose();
- _M_data(__tmp);
- _M_capacity(__requested_capacity);
- }
-#endif
+ this->_S_copy(_M_local_data(), _M_data(), __length + 1);
+ _M_destroy(__capacity);
+ _M_data(_M_local_data());
}
+#if __cpp_exceptions
+ else if (__length < __capacity)
+ try
+ {
+ pointer __tmp
+ = _Alloc_traits::allocate(_M_get_allocator(), __length + 1);
+ this->_S_copy(__tmp, _M_data(), __length + 1);
+ _M_dispose();
+ _M_data(__tmp);
+ _M_capacity(__length);
+ }
+ catch (const __cxxabiv1::__forced_unwind&)
+ { throw; }
+ catch (...)
+ { /* swallow the exception */ }
+#endif
}
template<typename _CharT, typename _Traits, typename _Alloc>
@@ -983,7 +984,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// 2968. Inconsistencies between basic_string reserve and
// vector/unordered_map/unordered_set reserve functions
// P0966 reserve should not shrink
- if (__res <= capacity())
+ if (__res <= __capacity)
{
if (!_M_rep()->_M_is_shared())
return;
@@ -992,10 +993,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__res = __capacity;
}
- const allocator_type __a = get_allocator();
- _CharT* __tmp = _M_rep()->_M_clone(__a, __res - this->size());
- _M_rep()->_M_dispose(__a);
- _M_data(__tmp);
+ const allocator_type __a = get_allocator();
+ _CharT* __tmp = _M_rep()->_M_clone(__a, __res - this->size());
+ _M_rep()->_M_dispose(__a);
+ _M_data(__tmp);
}
template<typename _CharT, typename _Traits, typename _Alloc>
@@ -1039,7 +1040,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// The standard places no restriction on allocating more memory
// than is strictly needed within this layer at the moment or as
- // requested by an explicit application call to reserve().
+ // requested by an explicit application call to reserve(n).
// Many malloc implementations perform quite poorly when an
// application attempts to allocate memory in a stepwise fashion
@@ -1176,23 +1177,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template<typename _CharT, typename _Traits, typename _Alloc>
void
basic_string<_CharT, _Traits, _Alloc>::
- _M_shrink(size_type __requested_capacity)
+ reserve()
{
- const size_type __capacity = capacity();
- if (__requested_capacity < __capacity || _M_rep()->_M_is_shared())
- {
- if (__requested_capacity > __capacity)
- // unshare, but keep same capacity
- __requested_capacity = __capacity;
- // Make sure we don't shrink below the current size
- else if (__requested_capacity < this->size())
- __requested_capacity = this->size();
- const allocator_type __a = get_allocator();
- _CharT* __tmp
- = _M_rep()->_M_clone(__a, __requested_capacity - this->size());
- _M_rep()->_M_dispose(__a);
- _M_data(__tmp);
- }
+ if (length() < capacity() || _M_rep()->_M_is_shared())
+ try
+ {
+ const allocator_type __a = get_allocator();
+ _CharT* __tmp = _M_rep()->_M_clone(__a);
+ _M_rep()->_M_dispose(__a);
+ _M_data(__tmp);
+ }
+ catch (const __cxxabiv1::__forced_unwind&)
+ { throw; }
+ catch (...)
+ { /* swallow the exception */ }
}
template<typename _CharT, typename _Traits, typename _Alloc>
