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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

First, thanks a lot for working on this, I am really excited that this is gaining traction.

I would recommend to investigate, whether it is really necessary to forbid SSO during constant evaluation. AFAIK switching of union members is totally fine at compile time. The main issue is to ensure that the underlying array is properly initialized. See https://github.com/microsoft/STL/pull/1735#discussion_r674285711 for a discussion on this (and how it is solved in MSVC). This would also really benefit the whole PR as that special casing is quite difficult.

While it is preexisting, there is a lot of invalid stuff happening here. Especially the fact that `char_traits` are generally ignored is troubling.

I would like to note, that the `__begin_lifetime` function seems a bit like an overkill. An `if (is_constant_evaluated())` branch with a call to `char_traits::assign` is not that much of an overhead



================
Comment at: libcxx/include/string:1515-1517
+        if (__libcpp_is_constant_evaluated())
+            return true;
+        return bool(__r_.first().__s.__size_ & __short_mask);
----------------
Why are you disabling SSO for constant evaluation. Is there some unavoidable UB in the implementation?

Note that we are just reenabling SSO for MSVC constexpr string (https://github.com/microsoft/STL/pull/1735)

There are a whole host of technically necessary things one has to do but it is generally quite easy to centralize


================
Comment at: libcxx/include/string:1623
+        }
+        for (unsigned __i = 0; __i < __n_words; ++__i)
+            __a[__i] = 0;
----------------
Nitpic: This is the most ridiculous formatting I have ever seen. Is there anyone on this planet that does not immediately reads the brace below as the closing brace of this `for`


================
Comment at: libcxx/include/string:1624
+        for (unsigned __i = 0; __i < __n_words; ++__i)
+            __a[__i] = 0;
         }
----------------
Preexisting: This is not correct. All operations must go through `char_traits`, so this should be `char_traits::assign(__a, __n_words, value_type{})`


================
Comment at: libcxx/include/string:1821
+                for (size_t __i = 0; __i < __count; ++__i)
+                    ::new (&__p[__i]) value_type();
+        }
----------------
This is technically ill formed. Only `std::construct_at` and `std::ranges::construct_at` are allowed inside core constant expressions. I know clang does currently extend this if the call is reached through `std::allocator::allocate` but there is no guarantee this will stay

Moreover, this is also ill formed, because it must go through `char_traits::assign`, which better does a `construct_at` at compile time


================
Comment at: libcxx/include/string:1952
     pointer __p;
-    if (__reserve < __min_cap)
+    if (__reserve < __min_cap && !__libcpp_is_constant_evaluated())
     {
----------------
Ditto, should not be necessary


================
Comment at: libcxx/include/string:2391
                           __ms - 1;
     pointer __p = __alloc_traits::allocate(__alloc(), __cap+1);
+    __begin_lifetime(__p, __cap + 1);
----------------
NIt: please fix the formatting of `__cap+1`


================
Comment at: libcxx/include/string:1466
 #if _LIBCPP_STD_VER > 17
     _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
     bool starts_with(__self_view __sv) const _NOEXCEPT
----------------
Quuxplusone wrote:
> These existing C++20-only methods could all be `_LIBCPP_CONSTEXPR` instead of `_LIBCPP_CONSTEXPR_AFTER_CXX11`; but I think I'll open a new separate PR for that, so never mind.
I am pretty sure they should be plain `constexpr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598