[PATCH] D56997: Fix implementation of P0966 - string::reserve Should Not Shrink

[Andrew Luo via Phabricator]

andrewluo marked 3 inline comments as done.
andrewluo added inline comments.


================
Comment at: include/string:963
 
-    void reserve(size_type __res_arg);
+    _LIBCPP_INLINE_VISIBILITY void reserve(size_type __res_arg);
     _LIBCPP_INLINE_VISIBILITY void __resize_default_init(size_type __n);
----------------
EricWF wrote:
> This change is ABI breaking.
> 
> Previously `reserve` was explicitly instantiated in the dylib. After this change it is not.
> I'll think more about what the best way to handle this is.
One possibility would be to introduce a new define, for example _LIBCPP_INLINE_VISIBILITY_AFTER_CXX17.  libc++.so is compiled in C++11 dialect so these symbols would be included for pre-C++17, while those setting dialect C++2a (or newer) would get the inline version.

But if we eliminate the #if altogether then this would go away.


================
Comment at: include/string:3147
     size_type __sz = size();
+#if _LIBCPP_STD_VER > 17
+    if (__res_arg <= __cap)
----------------
EricWF wrote:
> EricWF wrote:
> > I think we need to re-implement `shrink_to_fit`  after this change, since it is currently implemented as `reserve(0)`. 
> Since the new behavior is also allowed prior to C++20, I wonder if it would be better to make this change unconditionally?
Good catch with shrink_to_fit.  I will fix that.

I was considering that as well.

My only concern was that older applications that may rely on using reserve to set the capacity (rather than using shrink_to_fit as is proper in C++20) might be affected by this change (behavior changes are more expected when you change the -std dialect options).  I'm not too concerned about this, but I'm not familiar with libc++ development practices.

If the consensus is that this is fine, I can make that change.  This also solves the issue with changing the ABI by making the function inline.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56997