[libcxx-commits] [PATCH] D73223: Partially inline basic_string copy constructor in UNSTABLE

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

This patch should contain the changes to the unstable extern template list.

Also, the added test should pass both before and after your change, correct?
That is: does this change introduce a behavioral change, or is it simply an optimization?

Otherwise, this patch LGTM.



================
Comment at: libcxx/include/string:1553
+    // Slow path for the (inlined) copy constructor.
+    // Always externally instantiated and not inlined.
+    // Asserts that __s is zero terminated.
----------------
This isn't always externally instantiated.
And it doesn't assert that __s is null terminated (nor can it)

What's this comment really trying to say?


================
Comment at: libcxx/include/string:1892
+template <class _CharT, class _Traits, class _Allocator>
+void basic_string<_CharT, _Traits, _Allocator>::__init_copy_ctor_external(
+    const value_type* __s, size_type __sz) {
----------------
It's a bit unfortunate that this function essentially duplicates `__init(char*, size_type)`.

Can you write a short comment as to why this provides benefit over simply calling `__init`?
The formulation is essentially the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73223