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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: daltenty.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks! Still a few comments and questions, but this is looking really good now.



================
Comment at: libcxx/include/string:1593-1605
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+    void __set_long_cap(size_type __s) _NOEXCEPT {
+        if (__libcpp_is_constant_evaluated())
+            __r_.first().__l.__cap_ = __s;
+        else
+            __r_.first().__l.__cap_  = __long_mask | __s;
+    }
----------------
I don't understand why this diff is required when running in `constexpr`. What happens if you remove it?


================
Comment at: libcxx/include/string:1632
+        size_type (&__a)[__n_words] = __r_.first().__r.__words;
+        if (__libcpp_is_constant_evaluated()) {
+            __r_.first() = __rep();
----------------
Is there any reason why we don't simply always do `__r_.first() = __rep()`? That would work both in `constexpr` and non-`constexpr`, and it seems to be pretty much equivalent: https://godbolt.org/z/T85f13ozT

And when that's done, perhaps we can even remove `__zero()` altogether?


================
Comment at: libcxx/include/string:1806-1807
     bool __addr_in_range(_Tp&& __t) const {
+        if (__libcpp_is_constant_evaluated())
+          return true;
         const volatile void *__p = _VSTD::addressof(__t);
----------------
This needs a comment!


================
Comment at: libcxx/include/string:3104-3106
+        if (__libcpp_is_constant_evaluated()) {
+            __grow_by_and_replace(__cap, 0, __sz, __pos, __n1, __n2, __s);
+            return *this;
----------------
Note to future me: I looked at this and it seems OK, no need to spend another 15 minutes validating.


================
Comment at: libcxx/include/string:3451
     bool __was_long, __now_long;
-    if (__target_capacity == __min_cap - 1)
+    if (!__libcpp_is_constant_evaluated() && __target_capacity == __min_cap - 1)
     {
----------------
Can we repace this by a call to `__fits_in_sso(__target_capacity)`? (or maybe `__target_capacity +/- 1`, please double-check)


================
Comment at: libcxx/test/support/constexpr_char_traits.h:130
 template <class _CharT>
 TEST_CONSTEXPR_CXX14 _CharT*
 constexpr_char_traits<_CharT>::assign(char_type* __s, size_t __n, char_type __a)
----------------
@daltenty The CI is currently failing in pretty bad ways on AIX. The compiler is being reported as `IBM Open XL C/C++ for AIX 17.1.0 (5725-C72, 5765-J18), clang version 13.0.0`, which we claim to support on our page.

I believe there might be some patches to the constant evaluator on top of Clang 13 that have not been cherry-picked to your internal branch. I suspect there are probably no reasonable workarounds to make `std::string` work inside `constexpr` on that compiler without fixing the compiler itself. Hence, I am unsure what's the best course of action here. I think what we should do is either:

1. Mark those tests as XFAIL temporarily on AIX (`LIBCXX-AIX-FIXME`). This has the downside that you'll lose coverage for `std::string` at runtime too. Otherwise,
2. Introduce a macro and `#ifdef` out only the `constexpr` part of the tests on AIX until the compiler is fixed or updated. I'm fine with doing that, but only if we know that it's a temporary state (< 6 months roughly).

Do you have thoughts on that?

Note to @philnik: Since I'm about to go OOO, if there is no resolution for this by EOW, feel free to land this with option (1) above (which is simplest) and we can always do (2) afterwards if that's the preferred solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598