[PATCH] Implement P0966 std::string::reserve should not shrink
Jonathan Wakely
jwakely@redhat.com
Tue Jan 22 14:46:00 GMT 2019
On 22/01/19 08:34 +0000, Andrew Luo wrote:
>Please see the attached patch. Still a work in progress, but mostly done.
>
>For reference:
>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0966r1.html
Thanks for the patch, Andrew. We'd need a changelog entry and
copyright assignment to use this, see
https://gcc.gnu.org/contribute.html
All patches requesting review need to be CC'd to the gcc-patches list
(and libstdc++ ones need to be sent to the libstdc++ list too).
I've added some comments inline below, and attached a proof-of-concept
showing how I'd rather implement this change.
>Index: libstdc++-v3/config/abi/pre/gnu.ver
>===================================================================
>--- libstdc++-v3/config/abi/pre/gnu.ver (revision 268105)
>+++ libstdc++-v3/config/abi/pre/gnu.ver (working copy)
>@@ -2238,6 +2238,14 @@
> # _Sp_make_shared_tag::_S_eq
> _ZNSt19_Sp_make_shared_tag5_S_eqERKSt9type_info;
>
>+ # basic_string::_M_request_capacity
>+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE19_M_request_capacityEm;
You'd need to export the Copy-On-Write versions of _M_request_capacity
too. It looks like the tests haven't been run for the COW string, as
I'd expect them to fail with an undefined reference to
_M_request_capacity, and because char/18654.cc relies on the shrink
behaviour to pass the tests. That's easy to fix though, just note the
actual capacity before the shrink request, and verify it hasn't
changed:
--- 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,12 +47,14 @@ 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 );
+ VERIFY( str.capacity() == cap );
}
}
>+ # basic_string::reserve() (C++2a)
>+ _ZNSs7reserveEv;
>+ _ZNSbIwSt11char_traitsIwESaIwEE7reserveEv;
>+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE7reserveEv;
> } GLIBCXX_3.4.25;
>
There are existing patterns which already match these new reserve()
overloads, so they would need to be altered to not match the new ones:
@@ -224,7 +224,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*;
@@ -1740,7 +1743,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]_*;
> # Symbols in the support library (libsupc++) have their own tag.
>Index: libstdc++-v3/include/bits/basic_string.h
>===================================================================
>--- libstdc++-v3/include/bits/basic_string.h (revision 268105)
>+++ libstdc++-v3/include/bits/basic_string.h (working copy)
>@@ -420,6 +420,9 @@
> void
> _M_erase(size_type __pos, size_type __n);
>
>+ void
>+ _M_request_capacity(size_type __requested_capacity);
I think it would be better to not add this new function. Currently we
have a single reserve(size_type) function which handles the growing
case, and the shrinking case. If the post-P0966 world means you have
to call separate functions for those cases (reserve to grow, and
shrink_to_fit to shrink) then it's suboptimal to use a single
implementation. No callers of reserve want the shrinking path, and no
callers of shrink_to_fit want the growing path.
See the attached patch for my preferred solution. It makes the
definition of reserve(size_type) smaller, and hopefully faster, and
more likely to be inlined.
This has another minor advantage, which is that shrink_to_fit() can
now work when -fno-exceptions is used, if the string fits in the SSO
buffer (previously it always did nothing for -fno-exceptions just
because it was simplest to implement as a call to reserve(0)).
> public:
> // Construct/copy/destroy:
> // NB: We overload ctors in some cases instead of using default
>@@ -1014,8 +1017,21 @@
> * data.
> */
> void
>+#if __cplusplus > 201703L
>+ reserve(size_type __res_arg);
>+#else
> reserve(size_type __res_arg = 0);
>+#endif
>
>+#if __cplusplus > 201703L
>+ /**
>+ * Deprecated function, added for compatibility
>+ */
We want to use the [[deprecated]] attribute, not a comment. And there
doesn't seem to be any point in only conditionally making the split
from one reserve function into two overloaded functions. Since you've
changed the behaviour of reserve(0) unconditionally, why not just
declare the reserve() member unconditionally too, instead of limiting
it to C++17? That means we can issue a deprecation warning for all
modes.
>+ void
>+ reserve()
>+ { }
It's also unfortunate to have to add new symbols to the shared library
for a deprecated function that will be removed at some point.
I'm not sure what to do about that. For now my patch doesn't remove
the default argument, but we could do this:
@@ -1014,7 +1032,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+
+ __attribute__((__always_inline__))
+ __attribute__((__deprecated__("use shrink_to_fit() or reserve(0)")))
+ void reserve() { }
/**
* Erases the string, making it empty.
@@ -3979,7 +4006,11 @@ _GLIBCXX_END_NAMESPACE_CXX11
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+
+ __attribute__((__always_inline__))
+ __attribute__((__deprecated__("use shrink_to_fit() or reserve(0)")))
+ void reserve() { this->reserve(0); }
/**
* Erases the string, making it empty.
The changes to the gnu.ver linker script shown above are still needed
here, to prevent the new overloads being exported with the wrong
symbol versions. But the always_inline attributes mean we don't need
to export new symbols.
For the COW string it still calls reserve(0) to un-share the string.
But there's a high level problem with this change: it makes it awkward
to do shrink_to_fit in C++98 mode. There's no shrink_to_fit() member
for C++98, and you're removing the reserve(0) behaviour. Usually we'd
implement resolutions for LWG issues in all relevant -std modes, but
in this case I'm not sure we want to do that. However, because
std::string and std::wstring are explicitly instantiated in the
library, it's not easy to provide different behaviour for -std=c++98
and other modes.
Maybe the answer is for the new zero-argument reserve() overload to do
shrink-to-fit in C++98 mode. By marking it always_inline we can give
it different behaviour in C++98 and later modes, because the explicit
instantiation in the library won't be used for that function.
>@@ -951,16 +963,11 @@
> 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);
>- }
>+ // P0966 reserve should not shrink
>+ if (__res <= capacity())
>+ return;
You're removing the property that calling reserve on a COW string
guarantees it is no longer shared. I think we need to keep that (as my
patch does).
Given that this change (either your version, or mine) alters the
semantics of std::string in all -std modes, I don't think we can make
this change now. It will have to wait for GCC 10, so that the changes
have more time on trunk.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 11433 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190122/30611889/attachment.bin>
More information about the Libstdc++
mailing list