[libcxx-commits] [PATCH] D110598: [libc++] Implement P0980R1 (constexpr std::string)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this huge task!
In general I'm happy, but I'm not familiar enough with the internals of our string implementation to validate you didn't miss any important parts. Therefore I leave approval to somebody more familiar with string.

(This patch basically touches every function in string and thus "breaks" other patches for string. Would it be an idea to make some cleanup patches after this patch lands? Then we can use `_LIBCPP_HIDE_FROM_ABI` and `std::`.)



================
Comment at: libcxx/include/string:77
 template<class charT, class traits = char_traits<charT>, class Allocator = allocator<charT> >
 class basic_string
 {
----------------
Please update the synopsis.


================
Comment at: libcxx/include/string:957
     _LIBCPP_HIDE_FROM_ABI constexpr
     void resize_and_overwrite(size_type __n, _Op __op) {
       __resize_default_init(__n);
----------------
The test `test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp` can be updated by removing
`#if defined(__cpp_lib_constexpr_string) && __cpp_lib_constexpr_string >= 201907L`



================
Comment at: libcxx/include/string:1409
 #if _LIBCPP_STD_VER > 17
     constexpr _LIBCPP_INLINE_VISIBILITY
     bool starts_with(__self_view __sv) const noexcept
----------------
Nit: I would keep the order `LIBCPP_INLINE_VISIBILITY constexpr` for consistency with the other changes.


================
Comment at: libcxx/include/string:1460
+
+    static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 void __begin_lifetime(pointer __begin, size_type __n) {
+#if _LIBCPP_STD_VER > 17
----------------
Should these new functions be private members?


================
Comment at: libcxx/test/std/strings/basic.string/string.access/at.pass.cpp:12
 // const_reference at(size_type pos) const;
 //       reference at(size_type pos);
 
----------------
Please update the synopsis in all tests.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/max_size.pass.cpp:82
-  // static_assert(test());
-#endif
 
----------------
Why don't you test this in `constexpr`? The signature has changed in the paper.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_pointer_size.pass.cpp:53
-  {
-    typedef std::string S;
-    test(S(""), 0, "", 0, S(""));
----------------
Why is this part removed?


================
Comment at: libcxx/test/std/strings/basic.string/string.starts_with/starts_with.char.pass.cpp:19
 
-bool test() {
+TEST_CONSTEXPR_CXX20 bool test() {
   {
----------------
These functions are only available in C++20. (The same for `ends_with` and `contains`.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598