Fix implementation of P0966 - string::reserve Should Not Shrink

[EricWF]

include/string

967–971

This change is ABI breaking.

Previously reserve was explicitly instantiated in the dylib. After this change it is not.
I'll think more about what the best way to handle this is.

3172

I think we need to re-implement shrink_to_fit after this change, since it is currently implemented as reserve(0).

3172

Since the new behavior is also allowed prior to C++20, I wonder if it would be better to make this change unconditionally?

[andrewluo]

include/string

967–971

One possibility would be to introduce a new define, for example _LIBCPP_INLINE_VISIBILITY_AFTER_CXX17. libc++.so is compiled in C++11 dialect so these symbols would be included for pre-C++17, while those setting dialect C++2a (or newer) would get the inline version.

But if we eliminate the #if altogether then this would go away.

3172

Good catch with shrink_to_fit. I will fix that.

I was considering that as well.

My only concern was that older applications that may rely on using reserve to set the capacity (rather than using shrink_to_fit as is proper in C++20) might be affected by this change (behavior changes are more expected when you change the -std dialect options). I'm not too concerned about this, but I'm not familiar with libc++ development practices.

If the consensus is that this is fine, I can make that change. This also solves the issue with changing the ABI by making the function inline.

[andrewluo]

Fixed with the new diff

[EricWF]

include/string

967–971

Libc++ may not be compiled in the C++11 dialect. We shouldn't depend on that being the case.

What I think we want to do is something like this:

#ifndef _LIBCPP_BUILDING_LIBRARY
#define _LIBCPP_INLINE_VISIBILITY
#endif
void reserve(size_type __capacity_arg);

968

This needs an _LIBCPP_INLINE_VISIBILITY because we don't want this symbol going into the dylib. That would break backwards compatibility issues.

977–978

I think we should make this reserve call act the same as it did prior to C++2a. That is, it should call __request_capacity(0) or shink_to_fit()

3157

After some more thinking, I think it's best we leave the old behavior as-is. All of the existing standard libraries seem to honor the shrink request prior to C++20, and that provides mostly consistent behavior to users. For consistency alone I think it's probably better not to backport this extension.

[andrewluo]

Made some changes to keep behavior the same in older -std= dialects. We shouldn't overload the function to avoid code such as:

auto f(&::std::string::reserve); // Ambiguous post C++17

from breaking for std < 17 (will break if -std=c++2a however)

[mclow.lists]

In D56997#1367223, @andrewluo wrote:

Made some changes to keep behavior the same in older -std= dialects. We shouldn't overload the function to avoid code such as:

auto f(&::std::string::reserve); // Ambiguous post C++17

from breaking for std < 17 (will break if -std=c++2a however)

Users are not allowed to take the address of standard library member functions.
See [member.functions]/2 for a description of why - the function you want to take an address of may not exist.

[andrewluo]

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC). Unfortunately, I've seen many instances of code that relies on this implementation-specific behavior (particularly older code that uses bind instead of lambdas).

Anyways though, if you think we should go ahead and split the functions for all dialects anyways (C++2a as well as pre-C++2a) then I can revert that part of the changes...

[jwakely]

include/string

3157

For libstdc++ I think we have no real choice but to change it for all dialects. Our std::string::reserve(size_type) is exported from the shared library, so users will always get whatever behaviour the library exports, irrespective of their -std options. So we'll be changing reserve(size_type) to never shrink, unconditionally. So the consistency argument is lost.

[jwakely]

In D56997#1367316, @andrewluo wrote:

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC).

But not with libc++, where there were already two overloads. Changing that to support broken code which was already unsupported seems unnecessary.

[andrewluo]

In D56997#1367748, @jwakely wrote:

In D56997#1367316, @andrewluo wrote:

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC).

But not with libc++, where there were already two overloads. Changing that to support broken code which was already unsupported seems unnecessary.

Not in any released version of libc++, only after this change (which was made recently): https://reviews.llvm.org/D54992?id=175629

[andrewluo]

include/string

3157

Is it not possible to do the same as here in libstdc++:

Old version of reserve(size_type) is instantiated in the library (thus old code will still receive the old behavior) while marking the function inline for C++2a and forward (thus newer code will see the newer behavior)?

[andrewluo]

Split reserve again for all dialects

[andrewluo]

Just wanted to check to see if there are any other comments or if this looks good.

[curdeius]

This review should probably be abandoned, as it was implemented in D91778.

[ldionne]

Commandeering to abandon change, since this was done in D91778. Thanks for the heads up, Marek.

include/string

#endif  // _LIBCPP_DEBUG_LEVEL >= 2
     _LIBCPP_INLINE_VISIBILITY size_type max_size() const _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY size_type capacity() const _NOEXCEPT
         {return (__is_long() ? __get_long_cap()
                              : static_cast<size_type>(__min_cap)) - 1;}
 
     void resize(size_type __n, value_type __c);
     _LIBCPP_INLINE_VISIBILITY void resize(size_type __n) {resize(__n, value_type());}
 
+#if !defined(_LIBCPP_BUILDING_LIBRARY)
+    _LIBCPP_INLINE_VISIBILITY
+#endif
+#if _LIBCPP_STD_VER > 17
     void reserve(size_type __res_arg);
+#else
+    void reserve(size_type __res_arg = 0);
+#endif
+    _LIBCPP_INLINE_VISIBILITY void __request_capacity(size_type __capacity_arg);
     _LIBCPP_INLINE_VISIBILITY void __resize_default_init(size_type __n);
 
+#if _LIBCPP_STD_VER > 17
+    _LIBCPP_DEPRECATED
     _LIBCPP_INLINE_VISIBILITY
-    void reserve() _NOEXCEPT {reserve(0);}
+    void reserve() _NOEXCEPT {shrink_to_fit();}
+#endif
     _LIBCPP_INLINE_VISIBILITY
-    void shrink_to_fit() _NOEXCEPT {reserve();}
+    void shrink_to_fit() _NOEXCEPT {__request_capacity(0);}
     _LIBCPP_INLINE_VISIBILITY
     void clear() _NOEXCEPT;
     _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
     bool empty() const _NOEXCEPT {return size() == 0;}
 
     _LIBCPP_INLINE_VISIBILITY const_reference operator[](size_type __pos) const _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY reference       operator[](size_type __pos)       _NOEXCEPT;
 

 #ifdef _LIBCPP_BIG_ENDIAN
     return (__m <= ~__long_mask ? __m : __m/2) - __alignment;
 #else
     return __m - __alignment;
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+#if !defined(_LIBCPP_BUILDING_LIBRARY)
+inline
+#endif
 void
 basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __res_arg)
 {
-    if (__res_arg > max_size())
+#if (_LIBCPP_STD_VER > 17) && !defined(_LIBCPP_BUILDING_LIBRARY)
+    size_type __cap = capacity();
+    if (__res_arg <= __cap)
+        return;
+#endif
+    __request_capacity(__res_arg);
+}
+
+template <class _CharT, class _Traits, class _Allocator>
+inline void
+basic_string<_CharT, _Traits, _Allocator>::__request_capacity(size_type __capacity_arg)
+{
+    if (__capacity_arg > max_size())
         this->__throw_length_error();
     size_type __cap = capacity();
     size_type __sz = size();
-    __res_arg = _VSTD::max(__res_arg, __sz);
-    __res_arg = __recommend(__res_arg);
-    if (__res_arg != __cap)
+    __capacity_arg = _VSTD::max(__capacity_arg, __sz);
+    __capacity_arg = __recommend(__capacity_arg);
+    if (__capacity_arg != __cap)
     {
         pointer __new_data, __p;
         bool __was_long, __now_long;
-        if (__res_arg == __min_cap - 1)
+        if (__capacity_arg == __min_cap - 1)
         {
             __was_long = true;
             __now_long = false;
             __new_data = __get_short_pointer();
             __p = __get_long_pointer();
         }
         else
         {
-            if (__res_arg > __cap)
-                __new_data = __alloc_traits::allocate(__alloc(), __res_arg+1);
+            if (__capacity_arg > __cap)
+                __new_data = __alloc_traits::allocate(__alloc(), __capacity_arg+1);
             else
             {
             #ifndef _LIBCPP_NO_EXCEPTIONS
                 try
                 {
             #endif  // _LIBCPP_NO_EXCEPTIONS
-                    __new_data = __alloc_traits::allocate(__alloc(), __res_arg+1);
+                    __new_data = __alloc_traits::allocate(__alloc(), __capacity_arg+1);
             #ifndef _LIBCPP_NO_EXCEPTIONS
                 }
                 catch (...)
                 {
                     return;
                 }
             #else  // _LIBCPP_NO_EXCEPTIONS
                 if (__new_data == nullptr)
                     return;
             #endif  // _LIBCPP_NO_EXCEPTIONS
             }
             __now_long = true;
             __was_long = __is_long();
             __p = __get_pointer();
         }
         traits_type::copy(_VSTD::__to_raw_pointer(__new_data),
                           _VSTD::__to_raw_pointer(__p), size()+1);
         if (__was_long)
             __alloc_traits::deallocate(__alloc(), __p, __cap+1);
         if (__now_long)
         {
-            __set_long_cap(__res_arg+1);
+            __set_long_cap(__capacity_arg+1);
             __set_long_size(__sz);
             __set_long_pointer(__new_data);
         }
         else
             __set_short_size(__sz);
         __invalidate_all_iterators();
     }
 }

test/std/strings/basic.string/string.capacity/reserve.pass.cpp

 {
     typename S::size_type old_cap = s.capacity();
     S s0 = s;
     s.reserve();
     LIBCPP_ASSERT(s.__invariants());
     assert(s == s0);
     assert(s.capacity() <= old_cap);
     assert(s.capacity() >= s.size());
+#if TEST_STD_VER > 17
+    // reserve never shrinks as of P0966
+    assert(s.capacity() == old_cap);
+#endif
 }
 
 template <class S>
 void
 test(S s, typename S::size_type res_arg)
 {
     typename S::size_type old_cap = s.capacity();
     ((void)old_cap); // Prevent unused warning
     S s0 = s;
     if (res_arg <= s.max_size())
     {
         s.reserve(res_arg);
         assert(s == s0);
         assert(s.capacity() >= res_arg);
         assert(s.capacity() >= s.size());
 #if TEST_STD_VER > 17
-        assert(s.capacity() >= old_cap); // resize never shrinks as of P0966
+        // reserve never shrinks as of P0966
+        if (res_arg <= old_cap)
+            assert(s.capacity() == old_cap);
 #endif
     }
 #ifndef TEST_HAS_NO_EXCEPTIONS
     else
     {
         try
         {
             s.reserve(res_arg);
int main()
     s.erase(50);
     test(s, 5);
     test(s, 10);
     test(s, 50);
     test(s, 100);
     test(s, 1000);
     test(s, S::npos);
     }
+    {
+    S s;
+    test(s, 50);
+    test(s, 10);
+    test(s, 5);
+    }
+#if TEST_STD_VER > 17
+    {
+    S s;
+    s.reserve(100);
+    typename S::size_type old_cap = s.capacity();
+    s.reserve(5);
+    // reserve never shrinks as of P0966
+    assert(s.capacity() == old_cap);
+    s.reserve(old_cap - 1);
+    assert(s.capacity() == old_cap);
+    }
+#endif
     }
 #if TEST_STD_VER >= 11
     {
     typedef std::basic_string<char, std::char_traits<char>, min_allocator<char>> S;
     {
     S s;
     test(s);
 
#if TEST_STD_VER >= 11
     s.erase(50);
     test(s, 5);
     test(s, 10);
     test(s, 50);
     test(s, 100);
     test(s, 1000);
     test(s, S::npos);
     }
+    {
+    S s;
+    test(s, 50);
+    test(s, 10);
+    test(s, 5);
+    }
+#if TEST_STD_VER > 17
+    {
+    S s;
+    s.reserve(100);
+    typename S::size_type old_cap = s.capacity();
+    s.reserve(5);
+    // reserve never shrinks as of P0966
+    assert(s.capacity() == old_cap);
+    s.reserve(old_cap - 1);
+    assert(s.capacity() == old_cap);
+    }
+#endif
     }
 #endif
 }