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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/string:1935-1936
 {
+    if (__libcpp_is_constant_evaluated())
+        __zero();
     if (__reserve > max_size())
----------------
ldionne wrote:
> What if instead of doing this, we made sure to have the invariants of the string hold before calling `__init`, and then we'd "simply" have to check whether we already have a long string allocated at the beginning of `__init`?
I think we agreed that this is OK and we make sure that `__init` is only ever called inside constructors.


================
Comment at: libcxx/include/string:2878-2884
+    if (__libcpp_is_constant_evaluated()) {
+        if (__cap - __sz >= __n)
+            __grow_by_and_replace(__cap, 0, __sz, __pos, 0, __n, __s);
+        else
+            __grow_by_and_replace(__cap, __sz + __n - __cap, __sz, __pos, 0, __n, __s);
+        return *this;
+    }
----------------
ldionne wrote:
> I'd like to understand why this diff is needed. What happens if you remove it?
We may compare unrelated pointers with `__p + __pos <= __s && __s < __p + __sz`.


================
Comment at: libcxx/include/string:3093-3096
+        if (__libcpp_is_constant_evaluated()) {
+            __grow_by_and_replace(__cap, 0, __sz, __pos, __n1, __n2, __s);
+            return *this;
+        }
----------------
ldionne wrote:
> Same question here -- why is this required?
Same here - we may compare unrelated pointers.


================
Comment at: libcxx/include/string:4372-4373
     typename _String::size_type __rhs_sz = __rhs.size();
+    __r.__deallocate_constexpr();
     __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
     __r.append(__rhs.data(), __rhs_sz);
----------------
ldionne wrote:
> Could we investigate adding a private constructor that allows passing a capacity to pre-allocate, so this could be implemented like this:
> 
> ```
> using _String = basic_string<_CharT, _Traits, _Allocator>;
> typename _String::size_type __lhs_sz = __lhs.size();
> typename _String::size_type __rhs_sz = __rhs.size();
> _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()), __capacity_tag{}, __lhs_sz + __rhs_sz);
> __r.append(__lhs.data(), __lhs_sz);
> __r.append(__rhs.data(), __rhs_sz);
> return __r;
> ```
> 
> Can we please do this as a separate review? That should be easy to split out.
This is D123058 now.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/dtor_noexcept.pass.cpp:13
 
-// ~basic_string() // implied noexcept;
+// ~basic_string() // implied noexcept; // constexpr since C++20
 
----------------
ldionne wrote:
> Same thing here -- we don't have a runtime test for the string destructor outside of the static variable below. Please add one -- this can be done in the same review as the default ctor.
> 
> You could use a special allocator and check that `deallocate()` has been called after the string is destroyed.
This is now D123129.


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_find.last.not.of/string_view_size.pass.cpp:154-157
 //     typedef std::basic_string<char, std::char_traits<char>, min_allocator<char>> S;
 //     typedef std::string_view SV;
 //     test0<S, SV>();
 //     test1<S, SV>();
----------------
ldionne wrote:
> Can you try uncommenting this and seeing what happens?
Seems to just work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598