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

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet added inline comments.


================
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);
----------------
philnik wrote:
> AdvenamTacet wrote:
> > philnik wrote:
> > > AdvenamTacet wrote:
> > > > AdvenamTacet wrote:
> > > > > philnik wrote:
> > > > > > 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.
> > > > > Thx! I tried it, but `__grow_by` doesn't work because of  `_LIBCPP_HIDE_FROM_ABI_AFTER_V1`:
> > > > > ```
> > > > > undefined reference to `std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__grow_by[abi:v170000](unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long)'
> > > > > clang++: error: linker command failed with exit code 1 (use -v to see invocation)
> > > > > ```
> > > > > 
> > > > > Is there one more work-around?
> > > > > 
> > > > > It works when I remove `_LIBCPP_HIDE_FROM_ABI_AFTER_V1`, but I believe it's worse solution than a few more copy-pasted lines.
> > > > > Let me know what you think!
> > > > Hey @philnik, does it look ok for you? If yes, please, accept the review. If you want me to work on something, please, let me know how I should improve it.
> > > Where does that happen? The inline version should always be emitted, so there should never be a linker error with `[abi:v170000]`. IMO code duplication isn't the right thing here, since the code is quite complex and will get out of sync in the future.
> > Hey @philnik, I recreated this error on CI. Let me know if I used `_LIBCPP_SUPPRESS_DEPRECATED_POP` correctly and if you know how to fix it. I agree that code duplication is far from ideal, but I don't know how to avoid it, keeping everything other requested. Possibly I'm just incorrectly using a macro.
> Ah, I understand the problem now. I think we can just do
> ```
> #if _LIBCPP_ABI_VERSION >= 2 // We want to use the function in the dylib in ABIv1
>   _LIBCPP_HIDE_FROM_ABI
> #endif
> ```
> That should solve the problem.
Thank you for suggestions! It works!

But fact that we cannot use `_LIBCPP_HIDE_FROM_ABI_AFTER_V1` made me ask a question, if we are doing everything correctly, which led to realization that I don't remove `__grow_by` function from ABIv2. I fixed it by removing it from `_LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST`.

Unfortunately, `_LIBCPP_HIDE_FROM_ABI_AFTER_V1` still does not work.
Now I believe everything it correct and ready to land. I will land it Tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148693