[libcxx-commits] [PATCH] D72160: Optimize / partially inline basic_string copy constructor

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

There are a couple of places that now duplicate the body of `__init_long`. Would it make sense to deduplicate that code in some way?

Also, the plan is to make the  `__init_long` function externally instantiated. But that will be done in a follow-up change since it involves the ABI.

Otherwise, this LGTM.



================
Comment at: libcxx/include/string:801
 
+    // Optimization opportunity: do not externally instantiate the copy
+    // constructor, which inlines short string initialization. Long string
----------------
This comment is a weird place. It's probably better suited on `__init_long` itself.


================
Comment at: libcxx/include/string:804
+    // initialization is delegated to the (external) __init_long()method,
+    // which results in a 3X-4X speed up for SSO initialization.
     basic_string(const basic_string& __str);
----------------
Numbers never age well in comments.  I would put them in the commit message instead.


================
Comment at: libcxx/include/string:1556
 
+    void __init_long(const value_type* __s, size_type __sz);
+
----------------
The name of this function should express the intention that it's out-of-line and explicitly instantiated. 

Historically not much attention was paid to how `std::string` handled inlining and explicit instantiation, and now that we're fixing that I want to make it a clear part of any change.

How about `__init_long_out_of_line`, `__init_long_external`, or `__init_long_slow`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72160