On 30/07/20 15:29 +0100, Jonathan Wakely wrote:
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?
Here's the combined patch, based on your original with my proposed
simplifications applied.
commit 1ba08b8a7f52e64ea6ee07f7b8fb85f2adc15e33
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Thu Jul 30 15:29:39 2020
libstdc++: Implement P0966 std::string::reserve should not shrink
Remove ability for reserve(n) to reduce a string's capacity. Add a new
reserve() overload that makes a shrink-to-fit request, and make
shrink_to_fit() use that.
Co-authored-by: Jonathan Wakely <jwak...@redhat.com>
libstdc++-v3/ChangeLog:
2020-07-30 Andrew Luo <andrewluotechnolog...@outlook.com>
Jonathan Wakely <jwak...@redhat.com>
* config/abi/pre/gnu.ver (GLIBCXX_3.4): Use less greedy
patterns for basic_string members.
(GLIBCXX_3.4.29): Export new basic_string::reserve symbols.
* doc/xml/manual/status_cxx2020.xml: Update P0966 status.
* include/bits/basic_string.h (shrink_to_fit()): Call reserve().
(reserve(size_type)): Remove default argument.
(reserve()): Declare new overload.
[!_GLIBCXX_USE_CXX11_ABI] (shrink_to_fit, reserve): Likewise.
* include/bits/basic_string.tcc (reserve(size_type)): Remove
support for shrinking capacity.
(reserve()): Perform shrink-to-fit operation.
[!_GLIBCXX_USE_CXX11_ABI] (reserve): Likewise.
* testsuite/21_strings/basic_string/capacity/1.cc: Adjust to
reflect new behavior.
* testsuite/21_strings/basic_string/capacity/char/1.cc:
Likewise.
* testsuite/21_strings/basic_string/capacity/char/18654.cc:
Likewise.
* testsuite/21_strings/basic_string/capacity/char/2.cc:
Likewise.
* testsuite/21_strings/basic_string/capacity/wchar_t/1.cc:
Likewise.
* testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc:
Likewise.
* testsuite/21_strings/basic_string/capacity/wchar_t/2.cc:
Likewise.
diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 17aff5d907b..3ff7bb61002 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -223,7 +223,10 @@ GLIBCXX_3.4 {
_ZNSs6assignE[PRcjmvy]*;
_ZNSs6insertE[PRcjmvy]*;
_ZNSs6insertEN9__gnu_cxx17__normal_iteratorIPcSsEE[PRcjmvy]*;
- _ZNSs[67][j-z]*E[PRcjmvy]*;
+ _ZNSs6rbeginEv;
+ _ZNSs6resizeE[jmy]*;
+ _ZNSs7replaceE[jmy]*;
+ _ZNSs7reserveE[jmy];
_ZNSs7[a-z]*EES2_[NPRjmy]*;
_ZNSs7[a-z]*EES2_S[12]*;
_ZNSs12_Alloc_hiderC*;
@@ -290,7 +293,10 @@ GLIBCXX_3.4 {
_ZNSbIwSt11char_traitsIwESaIwEE6assignE[PRwjmvy]*;
_ZNSbIwSt11char_traitsIwESaIwEE6insertE[PRwjmvy]*;
_ZNSbIwSt11char_traitsIwESaIwEE6insertEN9__gnu_cxx17__normal_iteratorIPwS2_EE[PRwjmvy]*;
- _ZNSbIwSt11char_traitsIwESaIwEE[67][j-z]*E[PRwjmvy]*;
+ _ZNSbIwSt11char_traitsIwESaIwEE6rbeginEv;
+ _ZNSbIwSt11char_traitsIwESaIwEE7replaceEmm[PRm]*;
+ _ZNSbIwSt11char_traitsIwESaIwEE6resizeEm*;
+ _ZNSbIwSt11char_traitsIwESaIwEE7reserveEm;
_ZNSbIwSt11char_traitsIwESaIwEE7[a-z]*EES6_[NPRjmy]*;
_ZNSbIwSt11char_traitsIwESaIwEE7[a-z]*EES6_S[56]*;
_ZNSbIwSt11char_traitsIwESaIwEE12_Alloc_hiderC*;
@@ -1740,7 +1746,7 @@ GLIBCXX_3.4.21 {
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE6rbegin*;
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE6resize*;
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7replace*;
- _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserve*;
+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserveE[jmy];
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE8pop_back*;
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE9push_back*;
_ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE[7-9]_[MS]_*;
@@ -2304,6 +2310,11 @@ GLIBCXX_3.4.29 {
# std::from_chars
_ZSt10from_charsPKcS0_R[def]St12chars_format;
+ # basic_string::reserve()
+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserveEv;
+ _ZNSs7reserveEv;
+ _ZNSbIwSt11char_traitsIwESaIwEE7reserveEv;
+
} GLIBCXX_3.4.28;
# Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/doc/xml/manual/status_cxx2020.xml b/libstdc++-v3/doc/xml/manual/status_cxx2020.xml
index ade77cbb80b..b9ad03c720f 100644
--- a/libstdc++-v3/doc/xml/manual/status_cxx2020.xml
+++ b/libstdc++-v3/doc/xml/manual/status_cxx2020.xml
@@ -635,13 +635,12 @@ or any notes about the implementation.
</row>
<row>
- <?dbhtml bgcolor="#C8B0B0" ?>
<entry> <code>string::reserve</code> Should Not Shrink </entry>
<entry>
<link xmlns:xlink="http://www.w3.org/1999/xlink"; xlink:href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0966r1.html";>
P0966R1 </link>
</entry>
- <entry align="center"> </entry>
+ <entry align="center"> 11 </entry>
<entry />
</row>
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index bc0c256b65e..1c07aa0aa07 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -940,20 +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
- {
-#if __cpp_exceptions
- if (capacity() > size())
- {
- try
- { reserve(0); }
- catch(...)
- { }
- }
-#endif
- }
+ { reserve(); }
+#pragma GCC diagnostic pop
#endif
/**
@@ -985,7 +978,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+
+ /**
+ * Equivalent to shrink_to_fit().
+ */
+#if __cplusplus > 201703L
+ [[deprecated("use shrink_to_fit() instead")]]
+#endif
+ void
+ reserve();
/**
* Erases the string, making it empty.
@@ -3942,20 +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
- {
-#if __cpp_exceptions
- if (capacity() > size())
- {
- try
- { reserve(0); }
- catch(...)
- { }
- }
-#endif
- }
+ shrink_to_fit() noexcept
+ { reserve(); }
+#pragma GCC diagnostic pop
#endif
/**
@@ -3984,7 +3979,14 @@ _GLIBCXX_END_NAMESPACE_CXX11
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+
+ /// Equivalent to shrink_to_fit().
+#if __cplusplus > 201703L
+ [[deprecated("use shrink_to_fit() instead")]]
+#endif
+ void
+ reserve();
/**
* Erases the string, making it empty.
diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
index e370f439390..485b01d12b8 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -280,29 +280,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
basic_string<_CharT, _Traits, _Alloc>::
reserve(size_type __res)
{
- // Make sure we don't shrink below the current size.
- if (__res < length())
- __res = length();
-
const size_type __capacity = capacity();
- if (__res != __capacity)
- {
- if (__res > __capacity
- || __res > size_type(_S_local_capacity))
- {
- pointer __tmp = _M_create(__res, __capacity);
- this->_S_copy(__tmp, _M_data(), length() + 1);
- _M_dispose();
- _M_data(__tmp);
- _M_capacity(__res);
- }
- else if (!_M_is_local())
- {
- this->_S_copy(_M_local_data(), _M_data(), length() + 1);
- _M_destroy(__capacity);
- _M_data(_M_local_data());
- }
- }
+ // _GLIBCXX_RESOLVE_LIB_DEFECTS
+ // 2968. Inconsistencies between basic_string reserve and
+ // vector/unordered_map/unordered_set reserve functions
+ // P0966 reserve should not shrink
+ if (__res <= __capacity)
+ return;
+
+ pointer __tmp = _M_create(__res, __capacity);
+ this->_S_copy(__tmp, _M_data(), length() + 1);
+ _M_dispose();
+ _M_data(__tmp);
+ _M_capacity(__res);
}
template<typename _CharT, typename _Traits, typename _Alloc>
@@ -342,6 +332,41 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_set_length(length() - __n);
}
+ template<typename _CharT, typename _Traits, typename _Alloc>
+ void
+ basic_string<_CharT, _Traits, _Alloc>::
+ reserve()
+ {
+ if (_M_is_local())
+ return;
+
+ const size_type __length = length();
+ const size_type __capacity = _M_allocated_capacity;
+
+ if (__length <= 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 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>
void
basic_string<_CharT, _Traits, _Alloc>::
@@ -953,16 +978,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
basic_string<_CharT, _Traits, _Alloc>::
reserve(size_type __res)
{
- if (__res != this->capacity() || _M_rep()->_M_is_shared())
- {
- // Make sure we don't shrink below the current size
- if (__res < this->size())
- __res = this->size();
- 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 size_type __capacity = capacity();
+
+ // _GLIBCXX_RESOLVE_LIB_DEFECTS
+ // 2968. Inconsistencies between basic_string reserve and
+ // vector/unordered_map/unordered_set reserve functions
+ // P0966 reserve should not shrink
+ if (__res <= __capacity)
+ {
+ if (!_M_rep()->_M_is_shared())
+ return;
+
+ // unshare, but keep same capacity
+ __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);
}
template<typename _CharT, typename _Traits, typename _Alloc>
@@ -1006,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
@@ -1140,6 +1174,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return *this;
}
+ template<typename _CharT, typename _Traits, typename _Alloc>
+ void
+ basic_string<_CharT, _Traits, _Alloc>::
+ reserve()
+ {
+ 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>
typename basic_string<_CharT, _Traits, _Alloc>::size_type
basic_string<_CharT, _Traits, _Alloc>::
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
index 02f6bb2629e..0cd3381ccc1 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
@@ -138,11 +138,13 @@ void test01()
VERIFY( sz04 >= 100 );
str02.reserve();
sz03 = str02.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz03 < 100);
-#else
- VERIFY( sz03 == 0 );
-#endif
+ VERIFY( sz03 < sz04 );
+
+ // P0966: reserve should not shrink
+ str02.reserve(100);
+ sz03 = str02.capacity();
+ str02.reserve(sz03 - 1);
+ VERIFY( str02.capacity() == sz03 );
sz03 = str02.size() + 5;
str02.resize(sz03);
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
index 854c2902e6b..04658211992 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
@@ -35,11 +35,13 @@ void test01()
VERIFY( sz02 >= 100 );
str01.reserve();
sz01 = str01.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz01 < 100);
-#else
- VERIFY( sz01 == 0 );
-#endif
+ VERIFY( sz01 < sz02 );
+
+ // P0966: reserve should not shrink
+ str01.reserve(100);
+ sz01 = str01.capacity();
+ str01.reserve(sz01 - 1);
+ VERIFY( str01.capacity() == sz01 );
sz01 = str01.size() + 5;
str01.resize(sz01);
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
index e9dbf7f55f6..a499e6686df 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
@@ -47,9 +47,11 @@ void test01()
{
string str(i, 'x');
str.reserve(3 * i);
+ const size_type cap = str.capacity();
+ VERIFY( cap >= 3 * i );
str.reserve(2 * i);
- VERIFY( str.capacity() == 2 * i );
+ VERIFY( str.capacity() == cap );
str.reserve();
VERIFY( str.capacity() == i );
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
index 43ae4c6cd9d..dd8edc4e7a8 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
@@ -29,7 +29,7 @@ void test02()
std::string str01 = "twelve chars";
// str01 becomes shared
std::string str02 = str01;
- str01.reserve(1);
+ str01.reserve();
VERIFY( str01.capacity() >= 12 );
}
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
index 436ed59a3b5..c39f93c90dc 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
@@ -35,11 +35,13 @@ void test01()
VERIFY( sz02 >= 100 );
str01.reserve();
sz01 = str01.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz01 < 100);
-#else
- VERIFY( sz01 == 0 );
-#endif
+ VERIFY( sz01 < sz02 );
+
+ // P0966: reserve should not shrink
+ str01.reserve(100);
+ sz01 = str01.capacity();
+ str01.reserve(sz01 - 1);
+ VERIFY( str01.capacity() == sz01 );
sz01 = str01.size() + 5;
str01.resize(sz01);
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
index 374c27ea43d..e7426c16396 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
@@ -47,9 +47,11 @@ void test01()
{
wstring str(i, L'x');
str.reserve(3 * i);
+ const size_type cap = str.capacity();
+ VERIFY( cap >= 3 * i );
str.reserve(2 * i);
- VERIFY( str.capacity() == 2 * i );
+ VERIFY( str.capacity() == cap );
str.reserve();
VERIFY( str.capacity() == i );
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
index fac8adffcf4..a36386421bf 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
@@ -29,7 +29,7 @@ void test02()
std::wstring str01 = L"twelve chars";
// str01 becomes shared
std::wstring str02 = str01;
- str01.reserve(1);
+ str01.reserve();
VERIFY( str01.capacity() == 12 );
}
