Skip to content
This repository has been archived by the owner. It is now read-only.

c++2a: string::reserve still shrinks capacity (P0966) #45368

Closed
rprichard opened this issue Mar 31, 2020 · 3 comments
Closed

c++2a: string::reserve still shrinks capacity (P0966) #45368

rprichard opened this issue Mar 31, 2020 · 3 comments
Assignees

Comments

@rprichard
Copy link

@rprichard rprichard commented Mar 31, 2020

Bugzilla Link 45368
Resolution FIXED
Resolved on Nov 26, 2020 01:21
Version unspecified
OS Linux
CC @mkurdej,@dwblaikie,@ldionne,@mclow,@tambry
Fixed by commit(s) 841132efda2157c5f9e07cf31469470a6481ffd9

Extended Description

libc++ documents that it has implemented P0966R1[1][2], "string::reserve Should Not Shrink", but as far as I can tell, string::reserve still shrinks the capacity.

#include
#include <stdio.h>
int main() {
std::string foo;
foo.reserve(2000);
printf("%zu\n", foo.capacity());
foo.reserve(1000);
printf("%zu\n", foo.capacity());
return 0;
}

$ /x/llvm-upstream/stage1-install/bin/clang++ -stdlib=libc++ test.cpp && LD_LIBRARY_PATH=/x/llvm-upstream/stage1-install/lib ./a.out

2015
1007

[1] https://libcxx.llvm.org/cxx2a_status.html
[2] http://wg21.link/P0966R1

The P0966R1 change was implemented in D54992[3] / svn commit 347789[4].

[3] https://reviews.llvm.org/D54992
[4] https://reviews.llvm.org/rL347789

P0966R1 allows reserve() and reserve(0) to do different things, so they need to be overloads rather than use a default argument of 0, and the libc++ commit does split the function into two overloaded functions. libc++'s string::reserve(size_type) function still lowers the capacity, though. Its shrink_to_fit() still calls reserve(), which calls reserve(0).

http://eel.is/c++draft/string.capacity#itemdecl:6 reads:

constexpr void reserve(size_type res_arg);

Effects: A directive that informs a basic_­string of a planned change in size, so that the storage allocation can be managed accordingly. After reserve(), capacity() is greater or equal to the argument of reserve if reallocation happens; and equal to the previous value of capacity() otherwise. Reallocation happens at this point if and only if the current capacity is less than the argument of reserve().

The libc++ commit added a test that looks like it verifies that reserve doesn't shrink, but it doesn't really do that. In string.capacity/reserve.pass.cpp:

 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
+#endif

(I think the comment meant to say "reserve never shrinks" rather than "resize never shrinks"?)

This call to test() looks like it would catch the issue:

{
typedef std::string S;
...
{
S s(100, 'a');
s.erase(50);
test(s, 5);

We have a string, s with size() == 50 and capacity() >= 100. Calling reserve(5) should leave the capacity() >= 100 as of P0966R1, but libc++ shrinks the string to a little above 50. However, main() passes the string by-value to test(), and the copy constructor makes a new string that's shrunk-to-fit. i.e. AFAICT, the sections in main() that use erase() aren't testing the intended situation.

@rprichard
Copy link
Author

@rprichard rprichard commented Mar 31, 2020

assigned to @mkurdej

Loading

@mkurdej
Copy link
Member

@mkurdej mkurdej commented Nov 19, 2020

Thanks for a great bug report.
The patch is in a review: https://reviews.llvm.org/D91778.

Loading

@mkurdej
Copy link
Member

@mkurdej mkurdej commented Nov 26, 2020

Loading

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants