[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.

Everything here makes sense //except// the decision to make `s.reserve()` nonequivalent to `s.shrink_to_fit()`. I don't see any ABI issue with that, so that part of the patch is orthogonal to the rest — and it seems to be altering behavior that is both useful and cppreference-documented. I suggest doing everything else you're doing, but //not// that specific one-line diff, because I don't see its upside.



================
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.
----------------
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?]
```


================
Comment at: libcxx/include/string:990
     _LIBCPP_DEPRECATED_IN_CXX20 _LIBCPP_INLINE_VISIBILITY
-    void reserve() _NOEXCEPT {shrink_to_fit();}
+    void reserve() _NOEXCEPT { reserve(0); } // Implementation-specific behavior: we never shrink the string on reserve()
     _LIBCPP_INLINE_VISIBILITY
----------------
Why not? https://en.cppreference.com/w/cpp/string/basic_string/reserve specifically says that the zero-argument overload (which is deprecated in C++20) is //supposed// to be a synonym for `shrink_to_fit`. Why wouldn't we just do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117332