[libcxx-commits] [PATCH] D68617: partially inline copy constructor basic_string(const basic_string&[, allocator])

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__config:766
+#define _LIBCPP_HIDE_FROM_ABI_IN_UNSTABLE _LIBCPP_HIDE_FROM_ABI
+#define _LIBCPP_INLINE_IN_UNSTABLE inline
+#else
----------------
ldionne wrote:
> I'm really uncomfortable with piggy-backing on the fact that non-inline member functions with an explicit instantiation declaration won't be inlined into user code. We're also excluding this optimization from most builds which are not using the unstable ABI.
> 
> I think it's possible to do better by encoding the knowledge of when `__init_long()` was added to the shared library. As far as you're concerned, you could basically write code using `_LIBCPP_HAS_INIT_LONG_IN_DYLIB` (or whatever), and then vendors could go and define that macro when suitable for them.
> 
> We could also define all these `_LIBCPP_HAS_<function-name>_IN_DYLIB` macros in case we're using the unstable ABI. WDYT?
So, basically, my suggestion would be this:

1. `#define _LIBCPP_HAS_INIT_LONG_IN_DYLIB` in `__config` only when `_LIBCPP_ABI_UNSTABLE` is defined
2. Write the code in `<string>` such that it takes advantage of `__init_long` when it _is_in the dylib, and it doesn't when it's not (but in both cases the code should work).
3. I'll then go and also define `_LIBCPP_HAS_INIT_LONG_IN_DYLIB` when it makes sense based on the deployment target, and other vendors can do the same.

This seems like a clean solution where libc++ ends up being optimal in all cases where it can be. This also introduces a structured way of making this sort of change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68617