[libcxx-commits] [PATCH] D68840: [libc++][P0980] Marked member functions move/copy/assign of char_traits constexpr.

[Louis Dionne via Phabricator via libcxx-commits]

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

This is looking good. I especially like how the tests require only minimal change between the runtime and the compile-time version. I think this is what we should strive for pretty much every time we constexpr-ify something going forward.



================
Comment at: libcxx/include/__string:200
 
+// constexpr
+template <class _CharT>
----------------
This would be clearer (or something along those lines):

```
// constexpr version of the C functions of the same name
```



================
Comment at: libcxx/include/__string:203
+static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
+_CharT* __move_constexpr(_CharT* __s1, const _CharT* __s2, size_t __n) _NOEXCEPT
+{
----------------
`__memmove_constexpr`?


================
Comment at: libcxx/include/__string:216
+static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
+_CharT* __copy_constexpr(_CharT* __s1, const _CharT* __s2, size_t __n) _NOEXCEPT
+{
----------------
`__memcpy_constexpr`?


================
Comment at: libcxx/include/__string:224
+static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
+_CharT* __assign_constexpr(_CharT* __s, size_t __n, _CharT __a) _NOEXCEPT
+{
----------------
`__memset_constexpr`?


================
Comment at: libcxx/include/__string:656
 {
+    if (__n == 0) return __s1;
     char_type* __r = __s1;
----------------
Is that necessary?


================
Comment at: libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp:20
 
-int main(int, char**)
+TEST_CONSTEXPR_CXX20 bool test()
 {
----------------
Did you forget to include the change to `test_macros.h` (or whatever file) where you add `TEST_CONSTEXPR_CXX20`? I can't find it.

Also, I would suggest `TEST_CONSTEXPR_AFTER_CXX17`, to mirror the checks we usually make (which are always of the form `#if STD > 17`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68840