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

[Martijn Vels via Phabricator via libcxx-commits]

mvels marked an inline comment as done.
mvels added a comment.

Thanks for the elaborate comments Louis!

Sorry for the non working link, I messed with my fork, synced to a new machine and manage to wipe out the branch...

I discussed ABI implications with among others EricWf, and we should probably take it a step further in that we can't safely introduce new symbols and inlined dependencies to these symbols.

In this latest diff I included one possible solution to allow these ABI changes for UNSTABLE while keeping these a no-op for STABLE.

Ie, for STABLE we want to retain the ctors in the ABI, and not introduce new symbols, e.g.:

  basic_string(const basic_string&);
  
  _LIBCPP_HIDE_FROM_ABI
  __init_long(const basic_string&);

For UNSTABLE, we want to explicitly inline the ctor, and outline the __init_long();

  _LIBCPP_HIDE_FROM_ABI
  basic_string(const basic_string&);
  
  __init_long(const basic_string&);

This may deserve a more explicit (small) design doc or consensus. It is however a recurring issue for various methods we know to have a small, inlineable fast path which is now always a remote library call, for example, there are similar opportunities in assign, erase, etc, which does add up to significant speedups and CPU cost reductions .

Conditional ABI inclusion / exclusion macros in __config allow for a cleaner change in string than more noisy #ifdef ...UNSTABLE... blocks. This is just a suggested option, I imagine there are more ways to approach this. I don't believe there is a save option that offers forward and backwards ABI compatibility, so we can only change the inlining and add symbols (i.e., ABI break) in UNSTABLE.

As you highlight, any compilation with new headers linking against older libs or dylibs are a concern. More so as that these can pop up in deployment / runtime scenarios, which makes me firmly believe we should have some mechanism to apply these changes to UNSTABLE only with minimal noise in the string header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68617