[libcxx-commits] [PATCH] D117332: [libc++] Make sure basic_string::reserve never shrinks in all standard modes

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

In D117332#3261316 <https://reviews.llvm.org/D117332#3261316>, @ldionne wrote:

> I am reconsidering my decision now. Let's put ourselves in the shoes of a programmer that wrote code in C++03 when the standard did say "if n < size(), this is a non-binding shrink-to-fit request". They most likely used `str.reserve()` with the exact expectation that it was going to `shrink_to_fit`.
>
> API consistency is relevant for new code being written, however new code being written won't use `reserve()` because it's deprecated. So my opinion is now that if we change `reserve()` to never shrink, we're making the wrong tradeoff - existing code may change performance characteristics, and new code won't benefit from the consistency because new code won't use `reserve()` anyways.
>
> Thoughts? If you agree, I'll turn this patch around and make `reserve(n)` never shrink in all standard modes, but I won't touch `reserve()` (so that it shrinks to fit in all standard modes).

Sounds like I convinced you right as you convinced me? 😛 I think part of my confusion is that there are like three different things we might be worried about:
(A) Behavior change between `s.reserve()` compiled with Clang 14 `-std=c++17` versus the same source code compiled with Clang 14 `-std=c++20` (which leads to ODR violation when linked together).
(B) Behavior change between C++17 `s.reserve()` compiled with Clang 13, versus the same source code compiled with Clang 14.
(C) Behavior change between C++17 `s.reserve()` compiled with Clang 13 `-O0` and linked against the Clang 13 dylib, versus the same object file linked against the Clang 14 dylib.
(D) Behavior change between `s.reserve()` (compiled with Clang 14 in any mode), versus `s.reserve(0)` (ditto).

Prior to D91778 <https://reviews.llvm.org/D91778>, libc++ used the default-argument implementation (Default arguments are the devil!), so a source-level call to `s.reserve()` in old code would have been secretly calling `basic_string::reserve(0)`.

What @rsmith was complaining about was (A). Your original proposal (and what I was agreeing to when I clicked "Accept" :)) fixes (A) and (D) but breaks (B) and (C).
My proposal to leave C++20 `.reserve()` as shrink-to-fit (and what you just offered to do, IIUC) fixes (A) and (B) but breaks (C) and (D).

I think we cannot avoid breaking (C), because the dylib //must// implement the C++20 behavior for `basic_string::reserve(0)`, so a compiled call to `basic_string::reserve(0)` must not shrink, and in Clang 13 `s.reserve()` was secretly a call to `basic_string::reserve(0)`.

Anyway, you can go with the solution I suggested or the solution I accepted — I guess I have no call to be unhappy either way. ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117332/new/

https://reviews.llvm.org/D117332