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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked an inline comment as done.
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:
> 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.



================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.explicit_instantiation.sh.cpp:12
+// a regression test for the bug that was reported at https://stackoverflow.com/q/69520633/627587
+// and https://seedcentral.apple.com/sm/feedback_collector/radar/85053279.
+
----------------
Quuxplusone wrote:
> FYI, this link isn't reachable from outside Apple. (What ever happened to the `rdar://` URL scheme, anyway? :))
Hmm, I was pretty sure this was publicly accessible if you have a developer account? This is supposed to be the public version of the radar link, which is private.

I'd be keen to leave the link there since I think it should be accessible if you have an Apple developer account. If not, it will probably become accessible after the Xcode open beta is done. In all cases, it should be no less accessible than a `rdar://` link.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115656