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

[Eric Fiselier via Phabricator]

EricWF 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);
----------------
andrewluo wrote:
> 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.
Libc++ may not be compiled in the C++11 dialect. We shouldn't depend on that being the case.

What I think we want to do is something like this:

```
#ifndef _LIBCPP_BUILDING_LIBRARY
#define _LIBCPP_INLINE_VISIBILITY
#endif
void reserve(size_type __capacity_arg);
```


================
Comment at: include/string:964
     void reserve(size_type __res_arg);
+    void __request_capacity(size_type __capacity_arg);
     _LIBCPP_INLINE_VISIBILITY void __resize_default_init(size_type __n);
----------------
This needs an `_LIBCPP_INLINE_VISIBILITY` because we don't want this symbol going into the dylib. That would break backwards compatibility issues.


================
Comment at: include/string:968
     _LIBCPP_INLINE_VISIBILITY
     void reserve() _NOEXCEPT {reserve(0);}
     _LIBCPP_INLINE_VISIBILITY
----------------
I think we should make this reserve call act the same as it did prior to C++2a. That is, it should call `__request_capacity(0)` or `shink_to_fit()`


================
Comment at: include/string:3144
+    size_type __cap = capacity();
+    if (__res_arg <= __cap)
+        return;
----------------
After some more thinking, I think it's best we leave the old behavior as-is. All of the existing standard libraries seem to honor the shrink request prior to C++20, and that provides mostly consistent behavior to users. For consistency alone I think it's probably better not to backport this extension.


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

https://reviews.llvm.org/D56997