Here's an updated diff. Still a work in progress, ran the tests for
strings/capacity in both modes, passing now. I am running all the tests in
both modes as well, but will take a while to get results. In the meanwhile I
thought I'd send this out...
Also, that code works on all released versions of libc++. Only in the past
month or so was that overload added, also for a (slightly flawed)
implementation of P0966. Anyways, I removed the #ifs - I get your point that
there's no need to preserve backwards compatibility with non-comforming code,
and I think any issues with reserve specifically will be rather rare (and easy
to fix). (Although, I should point out that push_back is not overloaded if you
select -std=c++98, only -std=c++11 or later, but that's besides the point).
Thanks,
-Andrew
-----Original Message-----
From: Jonathan Wakely <jwak...@redhat.com>
Sent: Wednesday, January 23, 2019 3:26 AM
To: Andrew Luo <andrewluotechnolog...@outlook.com>
Cc: libstd...@gcc.gnu.org
Subject: Re: [PATCH] Implement P0966 std::string::reserve should not shrink
On 23/01/19 00:44 +0000, Andrew Luo wrote:
>Thanks for the input. I will make the suggested changes.
>
>I didn't add a changelog entry yet (which I was planning to do - just wanted
>to send this out before to get some feedback on the code first).
Understood, I'm just making sure you're aware of the process.
>I will also take care of the copyright assignment (should I send an email to
>g...@gcc.gnu.org or are you the maintainer that is taking care of this
>contribution?)...
That will probably be me. I'll send you the form to start the copyright
assignment.
>Next patch I will send to both libstdc++ and gcc-patches as well. Somehow I
>missed that.
Thanks.
>One possibility to avoid changing the behavior for pre-C++2a code is to put
>the check in a "#if __cplusplus > 201703L" block and force reserve(size_type)
>to be inline (if we want to go this route, I think the helper function
>_M_request_capacity would have to stay, as reserve could shrink in C++17 mode
>or earlier).
>Other than that, with my current patch, reserve() still works as previously,
>however reserve(0) has changed (I don't know how common it was to use
>reserve(0) instead of simply reserve())...
Well as you noted, your patch made it a no-op (more below).
>As for the split of reserve - wouldn't this kind of code break in previous
>-std modes:
>
>auto f(&::std::string::reserve); // Ambiguous post C++17
Already undefined, as you noted in the later reply. More on that below.
>As for deprecation, I will add it to the no-arg overload. Regarding giving
>the deprecation notice for all dialects, technically speaking, it was only
>deprecated in C++2a so should someone receive that deprecation message if they
>are compiling with -std=c++98, for example? I don't know what the general
>policies around this are for GCC/libstdc++.
Yes, you're right it's only deprecated for C++2a. My thinking was that if it's
going away at some point in future then it's useful to get a warning saying so,
period.
But we don't do that for std::auto_ptr in C++98 even though it's deprecated in
C++11. So maybe only add the attribute for C++2a, which means it can use C++11
attribute syntax:
#if__cplusplus > 201703L
[[deprecated("use shrink_to_fit() instead")]] #endif
On 23/01/19 03:41 +0000, Andrew Luo wrote:
>Actually, reserve() doesn't currently work as reserve(0) previously, I
>missed a line there (should have shrink_to_fit() in the body...)
Oh, I missed that the proposal says the new overload is a shrink-to-fit request
rather than no-op.
Calling shrink_to_fit() won't work for C++98 mode, because it isn't declared.
It could be a no-op for C++98 (it's only a non-binding request after all), or
we could add a new _M_shrink function that
shrink_to_fit() and reserve() call. I'm still reluctant to add a new exported
function (which means four new symbols) for a deprecated feature that already
exists as shrink_to_fit() but maybe that's the best option.
>Also, how do you run the tests for the COW string? I ran make check in the
>libstdc++-v3 folder and everything passes...
make check RUNTESTFLAGS=--target_board=unix/-D_GLIBCXX_USE_CXX11_ABI=0
And you can add other options, e.g. to check in C++98 mode:
RUNTESTFLAGS=--target_board=unix/-D_GLIBCXX_USE_CXX11_ABI=0/-std=gnu++98
See https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.run
for more info.
On 23/01/19 06:17 +0000, Andrew Luo wrote:
>Minor correction, someone kindly pointed out to me elsewhere that this code:
>
>auto f(&::std::string::reserve); // Ambiguous post C++17
>
>was never conforming to the C++ standard in the first place... as splitting
>the members was legal pre-C++2a regardless.
Right.
>That said, that piece of (non-conforming) code did work before (at least in
>MSVC + Dinkumware, I didn't check G++ + libstdc++ just yet, but unless G++ has
>some special enforcement of this in the compiler, it would seem to "work" from
>the library perspective...).
It won't compile with libc++ though, so it's already non-portable.
>I guess the question how much backward compatibility we want to have with
>non-conforming code such as the above. (Personal anecdote - I have seen this
>kind of stuff in older code, especially when used with bind and mem_fn).
That kind of code breaks routinely, e.g. using &std::vector::push_back which
was a single function in C++98 and overloaded in C++11.
Also, such use is incompatible with the always_inline attribute, so if we did
try to have different behaviour for older standards, using it via a pointer to
member would not necessarily get the C++98 semantics anyway. So I don't think
we can realistically retain the old behaviour, we just have to change it
unconditionally. We could keep a single overload just to make &string::reserve
compile, but I don't think that's worth giving up the chance to streamline
reserve(size_type) by removing the shrinking code path.
>But if you prefer, I can get rid of the #ifs.
Yes please.
Index: libstdc++-v3/ChangeLog
===================================================================
--- libstdc++-v3/ChangeLog (revision 268105)
+++ libstdc++-v3/ChangeLog (working copy)
@@ -1,3 +1,20 @@
+2019-01-18 Andrew Luo <andrewluotechnolog...@outlook.com>
+
+ Implement C++2a P0966
+ * config/abi/pre/gnu.ver (GLIBCXX_3.4.26): Export new symbols.
+ * doc/xml/manual/status_cxx2020.xml: Update to reflect P0966 is
+ implemented
+ * include/bits/basic_string.h: Implement P0966
+ * include/bits/basic_string.tcc: Implement P0966
+ * testsuite/21_strings/basic_string/capacity/1.cc: Update tests to
+ reflect new behavior per P0966
+ * 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
+
2019-01-18 Jonathan Wakely <jwak...@redhat.com>
PR libstdc++/87514
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)
@@ -224,7 +224,10 @@
_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*;
@@ -291,7 +294,10 @@
_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 @@
_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]_*;
@@ -2238,6 +2244,11 @@
# _Sp_make_shared_tag::_S_eq
_ZNSt19_Sp_make_shared_tag5_S_eqERKSt9type_info;
+ # basic_string::_M_shrink
+ _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE9_M_shrinkEm;
+ _ZNSs9_M_shrinkEm;
+ _ZNSbIwSt11char_traitsIwESaIwEE9_M_shrinkEm;
+
} GLIBCXX_3.4.25;
# Symbols in the support library (libsupc++) have their own tag.
Index: libstdc++-v3/doc/xml/manual/status_cxx2020.xml
===================================================================
--- libstdc++-v3/doc/xml/manual/status_cxx2020.xml (revision 268105)
+++ libstdc++-v3/doc/xml/manual/status_cxx2020.xml (working copy)
@@ -344,7 +344,7 @@
P0966R1
</link>
</entry>
- <entry align="center"> </entry>
+ <entry align="center"> 9.1 </entry>
<entry />
</row>
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_shrink(size_type __requested_capacity);
+
public:
// Construct/copy/destroy:
// NB: We overload ctors in some cases instead of using default
@@ -972,18 +975,8 @@
/// A non-binding request to reduce capacity() to size().
void
shrink_to_fit() noexcept
- {
-#if __cpp_exceptions
- if (capacity() > size())
- {
- try
- { reserve(0); }
- catch(...)
- { }
- }
+ { _M_shrink(0); }
#endif
- }
-#endif
/**
* Returns the total number of characters that the %string can hold
@@ -1014,8 +1007,19 @@
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+ /**
+ * Deprecated function, added for compatibility
+ */
+ __attribute__((__always_inline__))
+#if __cplusplus > 201703L
+ [[deprecated("use shrink_to_fit() instead")]]
+#endif
+ void
+ reserve()
+ { _M_shrink(0); }
+
/**
* Erases the string, making it empty.
*/
@@ -3940,18 +3944,8 @@
/// 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(...)
- { }
- }
+ { _M_shrink(0); }
#endif
- }
-#endif
/**
* Returns the total number of characters that the %string can hold
@@ -3979,8 +3973,19 @@
* data.
*/
void
- reserve(size_type __res_arg = 0);
+ reserve(size_type __res_arg);
+ /**
+ * Deprecated function, added for compatibility
+ */
+ __attribute__((__always_inline__))
+#if __cplusplus > 201703L
+ [[deprecated("use shrink_to_fit() instead")]]
+#endif
+ void
+ reserve()
+ { _M_shrink(0); }
+
/**
* Erases the string, making it empty.
*/
@@ -5104,6 +5109,9 @@
_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>
Index: libstdc++-v3/include/bits/basic_string.tcc
===================================================================
--- libstdc++-v3/include/bits/basic_string.tcc (revision 268105)
+++ libstdc++-v3/include/bits/basic_string.tcc (working copy)
@@ -280,29 +280,19 @@
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();
+ // _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;
- 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());
- }
- }
+ 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>
@@ -345,6 +335,40 @@
template<typename _CharT, typename _Traits, typename _Alloc>
void
basic_string<_CharT, _Traits, _Alloc>::
+ _M_shrink(size_type __requested_capacity)
+ {
+ 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 __capacity = capacity();
+ if (__requested_capacity < __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
+ }
+ }
+
+ template<typename _CharT, typename _Traits, typename _Alloc>
+ void
+ basic_string<_CharT, _Traits, _Alloc>::
resize(size_type __n, _CharT __c)
{
const size_type __size = this->size();
@@ -951,16 +975,25 @@
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 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>
@@ -1138,6 +1171,27 @@
return *this;
}
+ template<typename _CharT, typename _Traits, typename _Alloc>
+ void
+ basic_string<_CharT, _Traits, _Alloc>::
+ _M_shrink(size_type __requested_capacity)
+ {
+ 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);
+ }
+ }
+
template<typename _CharT, typename _Traits, typename _Alloc>
typename basic_string<_CharT, _Traits, _Alloc>::size_type
basic_string<_CharT, _Traits, _Alloc>::
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/1.cc
(working copy)
@@ -138,12 +138,14 @@
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);
sz04 = str02.size();
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/1.cc
(working copy)
@@ -35,12 +35,14 @@
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);
sz02 = str01.size();
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/18654.cc
(working copy)
@@ -47,9 +47,11 @@
{
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 );
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/char/2.cc
(working copy)
@@ -29,7 +29,7 @@
std::string str01 = "twelve chars";
// str01 becomes shared
std::string str02 = str01;
- str01.reserve(1);
+ str01.reserve();
VERIFY( str01.capacity() >= 12 );
}
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/1.cc
(working copy)
@@ -35,12 +35,14 @@
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);
sz02 = str01.size();
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/18654.cc
(working copy)
@@ -47,9 +47,11 @@
{
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 );
Index: libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
===================================================================
--- libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
(revision 268105)
+++ libstdc++-v3/testsuite/21_strings/basic_string/capacity/wchar_t/2.cc
(working copy)
@@ -29,7 +29,7 @@
std::wstring str01 = L"twelve chars";
// str01 becomes shared
std::wstring str02 = str01;
- str01.reserve(1);
+ str01.reserve();
VERIFY( str01.capacity() == 12 );
}
