[libcxx-commits] [PATCH] D148693: [libc++] Set correct size at the end of growing std::string

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/string:2284
+// but also not set size at all when string was short initially, leading to unpredictable size value.
+// It is not removed or changed to preserve old behavior and not break ABI.
 template <class _CharT, class _Traits, class _Allocator>
----------------
It's not like we would care about the old behavior otherwise.


================
Comment at: libcxx/include/string:1845
     _LIBCPP_CONSTEXPR_SINCE_CXX20
+    void __grow_by_without_replace(size_type __old_cap, size_type __delta_cap, size_type __old_sz,
+                   size_type __n_copy,  size_type __n_del,     size_type __n_add = 0);
----------------
ldionne wrote:
> I would be tempted to make this `_LIBCPP_HIDE_FROM_ABI` and not include it in the explicit instantiation list. So far, our explicit instantiation lists have only made our lives really hard with `std::string` (e.g. getting in the way of fixing bugs).
I don't really see the problem of adding this to the ABIv2 list. It's not like we will freeze ABIv2 any time soon, and we probably want to reassess what we export from the dylib and what we don't before doing that anyways.


================
Comment at: libcxx/include/string:1831-1833
     _LIBCPP_CONSTEXPR_SINCE_CXX20
     void __grow_by(size_type __old_cap, size_type __delta_cap, size_type __old_sz,
                    size_type __n_copy,  size_type __n_del,     size_type __n_add = 0);
----------------
AdvenamTacet wrote:
> philnik wrote:
> > We should mark this as `_LIBCPP_DEPRECATED_("use __grow_by_without_replace")` to avoid calling it accidentally.
> Cannot call deprecated function, so I had to copy-paste the code.
You can use `_LIBCPP_SUPPRESS_DEPRECATED_PUSH` and `_LIBCPP_SUPPRESS_DEPRECATED_POP` to ignore the diagnostic. IMO that's also what we should do to avoid code bloat.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148693