[libcxx-commits] [PATCH] D75211: Partially inline basic_string::operator=(const basic_string&)

[Martijn Vels via Phabricator via libcxx-commits]

mvels marked 6 inline comments as done.
mvels added inline comments.


================
Comment at: libcxx/include/__string:168
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__grow_by_and_replace(size_type, size_type, size_type, size_type, size_type, size_type, value_type const*)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__assign_no_alias<false>(value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__assign_no_alias<true>(value_type const*, size_type)) \
----------------
ldionne wrote:
> Now I'm thinking that splitting stable an unstable was a *really* good decision. I don't even have to worry about the effects of this change on the stable ABI.
Yes, I agree. BTW: we still need to make the ifdef under an ABI flag as Eric pointed out, but those sre mostly cosmetics.

Eric and I were discussing ideas Eric had about a future where we are not having to deal with all this ABI pain, and where we don't need these absolute splits. I do think we can get there (eventually), but until then, this is my preferred least worse solution :)


================
Comment at: libcxx/include/string:2212
+template <class _CharT, class _Traits, class _Allocator>
+template <bool __is_short>
+void basic_string<_CharT, _Traits, _Allocator>::__assign_no_alias(
----------------
ldionne wrote:
> I love this
Thanks, yes, it's an easy way to avoid branches, but also some other ideas that are in the pipeline that Eric and I discussed. 

For example, a 'grow' function that is overloaded for short / long. Our current growth pattern is a bit awkward as going from an SSO to a long string, we have 23 * 2 = 46 as a min cap, leading to an awkward first step up of 48 for length in range [23-47] (which also awkwardly doubles to 96, etc). Going from SSO to either 32 or 64 is a cleaner and cheaper step up and cache line friendly.




================
Comment at: libcxx/include/string:2292
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::operator=(const basic_string& __str)
 {
----------------
ldionne wrote:
> Are there homologous changes we should make to the various `assign` methods?
Yes, although I plan to have all these in separate incremental reviews. There may be some shuffling small adjustments of these functions going forward, but this being unstable, nothing we can not tinker with incrementally.

I'd also like to have my document shared soon (a mid week / weekend of skiing currently is in the way). For the 'regular' assign, there are some additional factors, for example, for assign(const value_type*), if we know the length at compile time, there is no need to exercise traits::length in the external code, we can special case those in a partial inlineable part (i.e., the submitted builtin_constant_p) which calls __assign_no_alias (as constant strings never alias). Ditto, if the string is at compile time known to be short, we can directly inline the assign, and have all other code elided, etc.

Like Eric imo these are best done incrementally 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75211