P0966R1 string::reserve() should not shrink

[NathanSWard]

Description

Addresses P0966R1 by preventing std::string::reserve() with arguments from shrinking the capacity. Also created deprecation warning, since std::string::reserve() with no arguments is deprecated in C++20.
Resolves #42

Checklist

• I understand README.md. I also understand that acceptance of
  community PRs will be delayed until the test and CI systems are online.
• If this is a feature addition, that feature has been voted into the
  C++ Working Draft.
• Identifiers in product code changes are properly _Ugly as per
  https://eel.is/c++draft/lex.name#3.1 .
• The STL builds successfully and all tests have passed (must be manually
  verified by an STL maintainer before CI is online, leave this unchecked for
  initial submission).
• These changes introduce no known ABI breaks (adding members, renaming
  members, adding virtual functions, changing whether a type is an aggregate
  or trivially copyable, etc.).
• These changes were written from scratch using only this repository and
  the C++ Working Draft as a reference (and any other cited standards).
  If they were derived from a project that's already listed in NOTICE.txt,
  that's fine, but please mention it. If they were derived from any other
  project (including Boost and libc++, which are not yet listed in
  NOTICE.txt), you must mention it here, so we can determine whether the
  license is compatible and what else needs to be done.

[CaseyCarter]

Obligatory "needs test coverage" comment. (There's not much you can do about this for now, but I thought you'd prefer some kind of feedback to no feedback.)

[StephanTLavavej]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CaseyCarter]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CaseyCarter]

All good modulo test coverage.

[StephanTLavavej]

Looks good to me. I'll do the manual work to add a behavioral test for reserve() and check this into Microsoft-internal git, followed by a merge here. Thanks!

[NathanSWard]

@StephanTLavavej and @CaseyCarter Should I squash the 2 commits into one?

[StephanTLavavej]

@StephanTLavavej and @CaseyCarter Should I squash the 2 commits into one?

No need, we'll squash when merging this. You're only one commit behind master and it rebases cleanly.

[StephanTLavavej]

Note that while this preserves behavior for reserve(), it is a behavioral change for reserve(res_arg). Previously, s.reserve(s.size()) would shrink from large to small (or large to empty), and now it doesn't. After thinking about it, I believe that this is acceptable for bincompat. Although the behavior is changing, C++17 always said it was a "non-binding" request, and regardless of which version is chosen in mix-and-match scenarios, the string's invariants are preserved (critically, we don't have to change functions A and B simultaneously in a coordinated way). If a program is expecting our C++17 behavior and it gets a C++20 instantiation (which doesn't shrink), the harm seems almost imaginary - we might consume extra memory, but that's it. If a program is expecting our C++20 behavior and gets a C++17 instantiation (which shrinks), there is more potential for harm - pointers/references/iterators could be invalidated, but the C++20 program could simply avoid calling reserve(res_arg) with an argument less than capacity(). I suppose we could avoid that by giving reserve(res_arg) extra default arguments, or maybe a default template argument, thus giving it a different mangled name, but that seems like unnecessary complexity for a probable non-issue. (And we can always do that later if necessary.)

I mention this only because ABI compat requires the highest level of care. Finishing the tests now.

[CaseyCarter]

We have tests, this is ready to go.

[StephanTLavavej]

Thanks! Here's the test that I ended up writing:

// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#define _SILENCE_CXX20_STRING_RESERVE_WITHOUT_ARGUMENT_DEPRECATION_WARNING

#include <assert.h>
#include <string>

#include <pmretvals.h>
using namespace std;

int main() {
    // N4830 [string.capacity]/9:
    // "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()."

    // [depr.string.capacity]/2:
    // "void reserve();
    // Effects: After this call, capacity() has an unspecified value greater than or equal to size().
    // [Note: This is a non-binding shrink to fit request. -end note]"

    // We've preserved our implementation's pre-C++20 behavior for reserve() without an argument,
    // performing a shrink to fit for empty strings in large mode (i.e. owning dynamically allocated memory).

    // Note that the exact values of capacity() are implementation-specific.

    const char* const large = "This is a very large string. Cute fluffy kittens. Meow! Purr!";

    string s(large);
    s.append(1000, '-');
    s.erase(55);

    assert(s.size() == 55);
    assert(s.capacity() == 1071);
    s.reserve(55); // reserve(res_arg) won't shrink from large to less-large
    assert(s.size() == 55);
    assert(s.capacity() == 1071);
    s.reserve(); // reserve() won't shrink from large to less-large
    assert(s.size() == 55);
    assert(s.capacity() == 1071);

    s.erase(4);

    assert(s.size() == 4);
    assert(s.capacity() == 1071);
#if _HAS_CXX20
    s.reserve(4); // reserve(res_arg) won't shrink from large to small in C++20
    assert(s.size() == 4);
    assert(s.capacity() == 1071);
#endif // _HAS_CXX20
    s.reserve(); // reserve() won't shrink from large to small
    assert(s.size() == 4);
    assert(s.capacity() == 1071);

    s.clear();

    assert(s.size() == 0);
    assert(s.capacity() == 1071);
#if _HAS_CXX20
    s.reserve(0); // reserve(res_arg) won't shrink from large to empty in C++20
    assert(s.size() == 0);
    assert(s.capacity() == 1071);
#endif // _HAS_CXX20
    s.reserve(); // reserve() WILL shrink from large to empty
    assert(s.size() == 0);
    assert(s.capacity() == 15);

#if !_HAS_CXX20
    s.assign(large);
    s.append(1000, '-');
    s.erase(4);

    assert(s.size() == 4);
    assert(s.capacity() == 1071);
    s.reserve(4); // reserve(res_arg) WILL shrink from large to small in our pre-C++20 mode
    assert(s.size() == 4);
    assert(s.capacity() == 15);

    s.assign(large);
    s.append(1000, '-');
    s.clear();

    assert(s.size() == 0);
    assert(s.capacity() == 1071);
    s.reserve(0); // reserve(res_arg) WILL shrink from large to empty in our pre-C++20 mode
    assert(s.size() == 0);
    assert(s.capacity() == 15);
#endif // !_HAS_CXX20

    s.assign("small");

    assert(s.size() == 5);
    assert(s.capacity() == 15);
    s.reserve(5); // reserve(res_arg) won't shrink when small
    assert(s.size() == 5);
    assert(s.capacity() == 15);
    s.reserve(); // reserve() won't shrink when small
    assert(s.size() == 5);
    assert(s.capacity() == 15);

    s.clear();

    assert(s.size() == 0);
    assert(s.capacity() == 15);
    s.reserve(0); // reserve(res_arg) won't shrink when small and empty
    assert(s.size() == 0);
    assert(s.capacity() == 15);
    s.reserve(); // reserve() won't shrink when small and empty
    assert(s.size() == 0);
    assert(s.capacity() == 15);

    // Test reserve(res_arg) with res_arg >= capacity():
    s.assign("cat");
    assert(s.size() == 3);
    assert(s.capacity() == 15);
    s.reserve(15); // same capacity when small, no change
    assert(s.size() == 3);
    assert(s.capacity() == 15);
    s.reserve(16); // increase capacity from small to large
    assert(s.size() == 3);
    assert(s.capacity() == 31);
    s.reserve(31); // same capacity when large, no change
    assert(s.size() == 3);
    assert(s.capacity() == 31);
    s.reserve(1000); // increase capacity "exactly" (with a bit of rounding)
    assert(s.size() == 3);
    assert(s.capacity() == 1007);
    s.reserve(1008); // increase capacity geometrically
    assert(s.size() == 3);
    assert(s.capacity() == 1510);

    return PM_TEST_PASS;
}