On 30/07/20 16:39 +0100, Jonathan Wakely wrote:
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.
I've now pushed that combined patch to master.
Sorry it took so long to integrate your changes, but thanks very much
for the contribution to GCC!
commit 1dbff6ffd71247a099028d4407d745dc0e5cf720
Author: Andrew Luo <andrewluotechnolog...@outlook.com>
Date: Thu Aug 6 19:35:43 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.
libstdc++-v3/ChangeLog:
2020-07-30 Andrew Luo <andrewluotechnolog...@outlook.com>
Jonathan Wakely <jwak...@redhat.com>
Implement C++20 P0966
* 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 b6ce76c1f20..b582f53e363 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]_*;
@@ -2309,6 +2315,11 @@ GLIBCXX_3.4.29 {
# std::__istream_extract(wistream&, wchar_t*, streamsize)
_ZSt17__istream_extractIwSt11char_traitsIwEEvRSt13basic_istreamIT_T0_EPS3_[ilx];
+ # basic_string::reserve()
+ _ZNSs7reserveEv;
+ _ZNSbIwSt11char_traitsIwESaIwEE7reserveEv;
+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserveEv;
+
} 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 52cecdd7ca1..e34b5c1fca3 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 75218a40610..a64b63a37fb 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..a60f2d62da8 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
@@ -136,13 +136,19 @@ void test01()
sz04 = str02.capacity();
VERIFY( sz04 >= sz03 );
VERIFY( sz04 >= 100 );
+#if __cplusplus <= 201703L
str02.reserve();
- sz03 = str02.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz03 < 100);
#else
- VERIFY( sz03 == 0 );
+ str02.shrink_to_fit(); // reserve is deprecated in C++20
#endif
+ sz03 = str02.capacity();
+ 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..cde253e1142 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
@@ -33,13 +33,19 @@ void test01()
size_type_s sz02 = str01.capacity();
VERIFY( sz02 >= sz01 );
VERIFY( sz02 >= 100 );
+#if __cplusplus <= 201703L
str01.reserve();
- sz01 = str01.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz01 < 100);
#else
- VERIFY( sz01 == 0 );
+ str01.shrink_to_fit(); // reserve is deprecated in C++20
#endif
+ sz01 = str01.capacity();
+ 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..13f65f57133 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,11 +47,17 @@ 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 );
+#if __cplusplus <= 201703L
str.reserve();
+#else
+ str.shrink_to_fit(); // reserve is deprecated in C++20
+#endif
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..1e87dde6d2e 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,11 @@ void test02()
std::string str01 = "twelve chars";
// str01 becomes shared
std::string str02 = str01;
- str01.reserve(1);
+#if __cplusplus <= 201703L
+ str01.reserve();
+#else
+ str01.shrink_to_fit(); // reserve is deprecated in C++20
+#endif
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..fa1584cc134 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
@@ -33,13 +33,19 @@ void test01()
size_type_s sz02 = str01.capacity();
VERIFY( sz02 >= sz01 );
VERIFY( sz02 >= 100 );
+#if __cplusplus <= 201703L
str01.reserve();
- sz01 = str01.capacity();
-#if _GLIBCXX_USE_CXX11_ABI
- VERIFY( sz01 < 100);
#else
- VERIFY( sz01 == 0 );
+ str01.shrink_to_fit(); // reserve is deprecated in C++20
#endif
+ sz01 = str01.capacity();
+ 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..fcdcd6abb14 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,11 +47,17 @@ 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 );
+#if __cplusplus <= 201703L
str.reserve();
+#else
+ str.shrink_to_fit(); // reserve is deprecated in C++20
+#endif
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..1ec140e61bf 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,11 @@ void test02()
std::wstring str01 = L"twelve chars";
// str01 becomes shared
std::wstring str02 = str01;
- str01.reserve(1);
+#if __cplusplus <= 201703L
+ str01.reserve();
+#else
+ str01.shrink_to_fit(); // reserve is deprecated in C++20
+#endif
VERIFY( str01.capacity() == 12 );
}
