Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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

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

rprichard opened this issue Mar 31, 2020 · 3 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@rprichard
Copy link
Contributor

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) 841132e

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
Contributor Author

assigned to @mkurdej

@mkurdej
Copy link
Member

mkurdej commented Nov 19, 2020

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

@mkurdej
Copy link
Member

mkurdej commented Nov 26, 2020

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

No branches or pull requests

2 participants