[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 accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:111-115
+- Calling ``std::basic_string::reserve()`` used to shrink the string if it could in all Standard modes.
+  Calling ``std::basic_string::reserve(capacity)`` used to shrink the string if it could in C++17 and before,
+  but not in C++20 and later. This behavior was changed so that both these calls never shrink the string
+  consistently in all Standard modes. This is a conforming implementation of the Standard, and this change
+  was done to avoid serious ODR violations when mixing code compiled in different Standard modes.
----------------
Quuxplusone wrote:
> Consider reflowing to fit in Phab's column count. :)
> "so that both these calls never shrink the string consistently in all Standard modes" — so they always shrink it inconsistently? 😛 Suggestion:
> ```
> - C++20 requires that ``std::basic_string::reserve`` never reduce the capacity
>   of the string. (For that, use ``shrink_to_fit``.) Prior to this release, libc++'s
>   ``std::basic_string::reserve(n)`` could reduce capacity in C++17 and before,
>   but not in C++20 and later. This caused ODR violations when mixing code compiled
>   under different Standard modes. After this change,
>   ``std::basic_string::reserve(n)`` never reduces capacity, even in C++17 
>   and before.
>   Also, after this change, `std::basic_string::reserve()`` is treated as a no-op,
>   rather than as a synonym for ``shrink_to_fit()``. [Arthur says: Why?]
> ```
> The rationale is that `reserve()` is supposed to be the same as `reserve(0)` -- actually in C++03 `reserve` is specified as a single overload with a default argument: `reserve(int = 0)`. If I make `reserve()` a synonym for `shrink_to_fit()`, it won't be equivalent to `reserve(0)` anymore.

`reserve()` is explicitly //supposed// to not-be-equivalent to `reserve(0)`, in C++20 and later... but, at the same time, it's also explicitly deprecated. Okay, I buy that. I'd explain it like this:
```
- C++20 requires that ``std::basic_string::reserve(n)`` never reduce the capacity
  of the string. (For that, use ``shrink_to_fit``.) Prior to this release, libc++'s
  ``std::basic_string::reserve`` could reduce capacity in C++17 and before,
  but not in C++20 and later. This caused ODR violations when mixing code compiled
  under different Standard modes. After this change, libc++'s
  ``std::basic_string::reserve`` never reduces capacity, even in C++17 
  and before.
  C++20 deprecates the zero-argument overload of ``std::basic_string::reserve()``,
  but specifically permits it to reduce capacity. libc++ does not take advantage
  of this permission: we treat ``reserve()`` as a synonym for ``reserve(0)``
  in all Standard modes.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117332