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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done and an inline comment as not done.
philnik added a comment.

Should there be another PR fixing any other ignored char_traits or should this be handled in this PR?



================
Comment at: libcxx/include/string:1515-1517
+        if (__libcpp_is_constant_evaluated())
+            return true;
+        return bool(__r_.first().__s.__size_ & __short_mask);
----------------
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.


================
Comment at: libcxx/include/string:1821
+                for (size_t __i = 0; __i < __count; ++__i)
+                    ::new (&__p[__i]) value_type();
+        }
----------------
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?


================
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
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598