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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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)
----------------
Mordante wrote:
> 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?
Indeed -- it's completely crazy.

IMO, we want all functions to be inline except when they are explicitly defined in and exported from the shared library. We've been moving towards that by defining functions in their class declarations more and more, and we should slowly keep moving towards that.

Instead of using `internal_linkage`, we could also look into using an inline namespace that gets bumped with every version of the library. That would reduce code bloat (due to `internal_linkage` functions being duplicated in each TU) and remove the need for using `_LIBCPP_HIDE_FROM_ABI` everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115656