[llvm-bugs] [Bug 45368] New: c++2a: string::reserve still shrinks capacity (P0966)

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=45368

            Bug ID: 45368
           Summary: c++2a: string::reserve still shrinks capacity (P0966)
           Product: libc++
           Version: unspecified
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: All Bugs
          Assignee: unassignedclangbugs at nondot.org
          Reporter: rprichard at google.com
                CC: llvm-bugs at lists.llvm.org, mclow.lists at gmail.com

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 <string>
#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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20200331/77a3b471/attachment-0001.html>