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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added inline comments.


================
Comment at: libcxx/include/string:1515-1517
+        if (__libcpp_is_constant_evaluated())
+            return true;
+        return bool(__r_.first().__s.__size_ & __short_mask);
----------------
philnik wrote:
> miscco wrote:
> > 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
> I don't know how it would be possible to know if it is a long or short string, because I have to check the bit. That is (AFAIK) not possible without knowing which member is active, for which I would have to know if it is a short or long string. If it is possible, let me know and I would be happy to implement it with SSO.
urgh, I totally forgot that the whole implementation is technically UB. Please disregard then


================
Comment at: libcxx/include/string:1821
+                for (size_t __i = 0; __i < __count; ++__i)
+                    ::new (&__p[__i]) value_type();
+        }
----------------
philnik wrote:
> miscco wrote:
> > 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
> Should I remove __begin_lifetime and modify char_traits::assign and (if there are any) other functions where it fails?
I mean the `__begin_lifetime` function would  be a complete nitpick on my side. Feels free to handle it as you like. 

The fact that `char_traits::assign` should go through `construct_at` when constant evaluated is a correctness necessity


================
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
----------------
philnik wrote:
> miscco wrote:
> > 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`
> Should I just include the fix in this PR, since it isn't important that it's fixed in trunk and is related to making string constexpr? 
Yeah, I would not wait on this. In the end it is something the maintainers should decide


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598