[libcxx-commits] [PATCH] D115656: [libc++] Fix wrongly non-inline basic_string::shrink_to_fit

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/string:3322
 template <class _CharT, class _Traits, class _Allocator>
 void
 basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __requested_capacity)
----------------
ldionne wrote:
> Mordante wrote:
> > Just curious but there are more similar functions not marked as `inline`. Are they also an issue and should they be fixed?
> > It would be odd if all these functions are "broken".
> > 
> > I wonder whether this is really the proper fix. Looking at `include/__string` it contains
> > `_Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \`
> > But no entry for `shrink_to_fit`.
> > Does adding `shrink_to_fit` to that list solve the issue.
> > 
> > (I didn't look for more omissions in `include/__string`.)
> > Just curious but there are more similar functions not marked as inline. Are they also an issue and should they be fixed?
> 
> These other non-`inline` functions are not marked with `_LIBCPP_HIDE_FROM_ABI` or `_LIBCPP_INLINE_VISIBILITY` AFAICT. Did you see some that were not inline but had `_LIBCPP_HIDE_FROM_ABI`?
> 
> > I wonder whether this is really the proper fix. Looking at `include/__string` it contains
> > ```_Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \
> > ```
> > But no entry for `shrink_to_fit`.
> 
> `reserve` is different, it is not marked with `_LIBCPP_HIDE_FROM_ABI`, so it doesn't get internal linkage. If we made it `_LIBCPP_HIDE_FROM_ABI`, we'd have the same issue.
> 
> > Does adding `shrink_to_fit` to that list solve the issue?
> 
> No it doesn't, since we'd be explicitly instantiating a function with internal linkage, so it wouldn't be available from outside the `string.cpp.o` TU. And these lists are used only for the `char` and `wchar_t` explicit instantiations anyways, so that wouldn't solve Chromium's issue, which is with their own custom `char16` type.
> 
I see.
The current requirements for when using `inline` and when not feel quite error-prone.
I wonder whether we need to add a better way to tag these functions. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115656