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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: smeenai, Quuxplusone.
Quuxplusone added a comment.

Reviewed about halfway before getting called away. Yeesh, this paper's changes are tedious — which means thank you for taking on the tedium!



================
Comment at: libcxx/include/__memory/compressed_pair.h:61-62
 
-
-  _LIBCPP_INLINE_VISIBILITY reference __get() _NOEXCEPT { return __value_; }
-  _LIBCPP_INLINE_VISIBILITY
-  const_reference __get() const _NOEXCEPT { return __value_; }
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 reference __get() _NOEXCEPT { return __value_; }
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 const_reference __get() const _NOEXCEPT { return __value_; }
 
----------------
For internal implementation details such as `__compressed_pair`, please be aggressive in adding `constexpr` (anywhere it's actually needed, of course). On these two lines, you can use `_LIBCPP_CONSTEXPR` instead of `_LIBCPP_CONSTEXPR_AFTER_CXX17`. The benefit is that on that far-future day where we drop support for C++03, we can just mechanically change all `_LIBCPP_CONSTEXPR` to `constexpr`; whereas we can't mechanically change `_LIBCPP_CONSTEXPR_AFTER_CXX17` to `constexpr` until we drop support for C++17!

For user-facing (public, standards-conforming) bits, of course we have to apply constexpr conservatively.


================
Comment at: libcxx/include/string:1117
     template <class _Tp>
-    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_AFTER_CXX17
     __enable_if_t
----------------
[FUD alert] I wonder whether there is any weird interaction between this unusual `_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS` specifier and `constexpr`. D29157 is the PR that introduced `_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS`; maybe @smeenai remembers what the deal was and whether `constexpr` is going to mess anything up.


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


================
Comment at: libcxx/include/string:1507
 
     _LIBCPP_INLINE_VISIBILITY void __clear_and_shrink() _NOEXCEPT;
 
----------------
Is `basic_string<char, char_traits<char>, CustomAllocator<char>>` supposed to be constexpr-friendly? or only `basic_string<char, char_traits<char>, allocator<char>>`? This may require asking LWG for their intent, because it's certainly not obvious from [p0980r1]. ("constexpr on library templates is meaningless" is a long-standing LWG issue.)

If we intend to support custom, constexpr-friendly, POCCA, allocators — then `__clear_and_shrink` will also need to be constexpr-friendly, and we need some new tests for this codepath.



================
Comment at: libcxx/include/string:1818-1820
+        if (__libcpp_is_constant_evaluated())
+                for (size_t __i {}; __i < __count; ++__i)
+                    new (&__p[__i]) value_type();
----------------
- Braces around multi-line branches, please (in this case, around the `if`)
- `size_t __i = 0` not `size_t __i {}`
- `::new` not `new` (ADL-proofing)
- I question the usefulness of `consteval` here; seems like a simple `constexpr` would do; but at least if you //keep// it `consteval`, then you certainly don't need to branch on `__libcpp_is_constant_evaluated()` because that will always be true. Actually, I think you call this on line 1956 in a non-constexpr context, so `consteval` should not compile, right?


================
Comment at: libcxx/include/string:1917
+    if (__libcpp_is_constant_evaluated())
+        __init(size_type{0}, {});
+    else
----------------
IIUC, this is the same as `__init(size_type(0), value_type());`. I'd prefer to see the latter.

It's kind of insane how we have so many overloads of `__init` and cavalierly call them with random arguments like `__init(__s, __n)`, `__init(__n, __c)`, where flipping the arguments does something radically different. In a perfect world we'd fix that (by giving each overload a nice distinct name). But that's not this PR. 


================
Comment at: libcxx/include/string:2363-2366
+    if (__libcpp_is_constant_evaluated()) {
+        if (__get_long_pointer() != nullptr)
+            __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
+    }
----------------
My understanding of this patch is that you're trying to make it an invariant that "`__libcpp_is_constant_evaluated()` implies `__is_long()`." In that case, you shouldn't need this diff, right?


================
Comment at: libcxx/test/std/input.output/iostream.format/quoted.manip/quoted_traits.verify.cpp:25
 template <class charT>
-struct test_traits {
-    typedef charT char_type;
+struct test_traits : std::char_traits<char> {
+  typedef charT char_type;
----------------
I recommend not inheriting from standard types in general. What causes this change to seem like a good idea? Is `test_traits` missing some members that `std::quoted` needs? Which ones?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598