[libcxx-commits] [PATCH] D131668: [libc++] Implement P2438R2 (std::string::substr() &&)

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/string:868-869
 
     _LIBCPP_CONSTEXPR_AFTER_CXX17
-    basic_string(const basic_string& __str, size_type __pos, size_type __n,
-                 const _Allocator& __a = _Allocator());
+    basic_string(const basic_string& __str, size_type __pos, size_type __n, const _Allocator& __a = _Allocator());
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
Does this need `_LIBCPP_HIDE_FROM_ABI`


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:35
+    S substr(std::move(orig), pos);
+    LIBCPP_ASSERT(orig.__invariants());
+    assert(substr == expected);
----------------
I guess this check does not check too much. Would it be worth to add a test in test/libcxx that checks for a long string the original string is "moved"?


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr.pass.cpp:110
+  test(S("lsaijeqhtrbgcdmpfkno"), 20, 0);
+  test(S("dplqartnfgejichmoskb"), 21, 0);
+}
----------------
Out of interest: none of the tests are using non-ascii characters. I guess it is irrelevant to what this particular test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668