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

[Zhihao Yuan via Phabricator via libcxx-commits]

lichray added inline comments.


================
Comment at: libcxx/include/array:504
+  static_assert(
+      is_copy_constructible_v<_Tp>,
+      "[array.creation]/1: to_array requires copy constructible elements.");
----------------
ldionne wrote:
> 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?
LWG raised this, probably because someone realized that built-in array initializer and `std::array` aggregate initialization allow types that "copy-constructs" from mutable references, so it seems not so motivated to ban those in `to_array`.


================
Comment at: libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp:25
+    MoveOnly mo;
+    std::to_array(mo); // expected-error {{no matching function}}
+  }
----------------
Not sure what is tested here.  `to_array` works with move-only elements (as you have tested in to_array.pass.cpp), but certainly won't work with anything that is not a built-in array :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69882