[libcxx-commits] [PATCH] D69882: [libcxx] Implement P0325: to_array from LFTS with updates.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: lichray.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/array:499
+template <typename _Tp, size_t _Size>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR auto to_array(_Tp (&__arr)[_Size]) {
+  static_assert(
----------------
You can use `constexpr` directly here (and elsewhere), since this is only enabled when `_LIBCPP_STD_VER > 17`.


================
Comment at: libcxx/include/array:499
+template <typename _Tp, size_t _Size>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR auto to_array(_Tp (&__arr)[_Size]) {
+  static_assert(
----------------
ldionne wrote:
> You can use `constexpr` directly here (and elsewhere), since this is only enabled when `_LIBCPP_STD_VER > 17`.
Please declare the return type of the function here, since otherwise `decltype(to_array(...))` will need to instantiate the function body in order to get the return type. It also makes for better code documentation, since the return type of `to_array` is definitely part of the public API.


================
Comment at: libcxx/include/array:504
+  static_assert(
+      is_copy_constructible_v<_Tp>,
+      "[array.creation]/1: to_array requires copy constructible elements.");
----------------
Did they really mean `is_constructible_v<T, T&>` in the paper? They say:

> Mandates: `is_array_v<T>` is `false` and `is_constructible_v<T, T&>` is `true`.

It seems like they should have said this instead?

> Mandates: `is_array_v<T>` is `false` and `is_copy_constructible_v<T>` is `true`.

Regardless, I think it would be better if you used `is_constructible_v<T, T&>` here, since that's what the paper says.

@lichray , why did you write the wording that way? Any reason?


================
Comment at: libcxx/test/std/containers/sequences/array/array.creation/to_array.pass.cpp:10
+// <array>
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
----------------
Can you please add the synopsis of the exact functions you're testing (so the two overloads of `to_array`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69882