[libc++] [P0325] Implement to_array from LFTS with updates.

[curdeius]

Summary

This patch implements https://wg21.link/P0325.
Please mind that at it is my first contribution to libc++, so I may have forgotten to abide to some conventions.

[zoecarver]

Thanks for the patch @curdeius! It looks great.

I also have an implementation here, D66103. We can use either (and maybe both sets of tests).

libcxx/include/array

501

I don't think you need the noexcept.

libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp

14

Is this needed?

25

You can get this with #include "MoveOnly.h".

libcxx/test/std/containers/sequences/array/array.creation/to_array.pass.cpp

22

You can use ASSERT_SAME_TYPE (from test_macros).

[curdeius]

Oh, I didn't know it, I only searched for to_array in master branch before starting.
How should I proceed? I let you take out of my patch what can be useful?
I see that you didn't touch feature test macros at all for instance. And as you said, you might add some tests I added and you seem not having (e.g. fail tests).

[ldionne]

Once you guys have figured out how you want to proceed, please say it here and abandon one of the two revisions. I'll review the one you decide to keep.

[zoecarver]

Let's use this patch. Both so that you can get something committed and because it has the feature test macros. Feel free to pull anything you want from my implementation.

Once we have more stuff in GitHub these things will happen less often (I hope).

[curdeius]

• Clean up to_array implementation.
• Add tests from D66103. Clean up.

[curdeius]

• Fix typos.

[curdeius]

Updated with tests from D66103. Answered issues from comments.

Sorry for the delay.

[curdeius]

I'm still not sure what to do about noexcept for to_array.
libstdc++ adds noexcept: https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/array#L361
MSVC STL does not add noexcept: https://github.com/microsoft/STL/blob/master/stl/inc/array#L429
It seems to be a low-hanging fruit to add noexcept(is_nothrow_copy_constructible_v<_Tp>) / noexcept(is_nothrow_move_constructible_v<_Tp>).
What do you think?

[ldionne]

libcxx/include/array

499

You can use constexpr directly here (and elsewhere), since this is only enabled when _LIBCPP_STD_VER > 17.

499

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.

504

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?

libcxx/test/std/containers/sequences/array/array.creation/to_array.pass.cpp

10

Can you please add the synopsis of the exact functions you're testing (so the two overloads of to_array)?

[lichray]

libcxx/include/array

504

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.

libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp

25

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 :)

[ldionne]

libcxx/include/array

504

Thanks a lot for the context. Ugh, bad design IMO, since now we have a subtle difference with anything that requires the usual copy-constructibility. But oh well. @curdeius let's match what the paper says.

[curdeius]

• Fix constexpr and return type.
• Fix tests.

[curdeius]

• Add synopsis.

[curdeius]

Adressed comments.

libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp

25

Oops, fixed it.

[curdeius]

Fix patch diff.

[lichray]

LGTM.

IIRC implementations can make functions "more" noexcept. If so then noexcept is welcome.

[curdeius]

• Readd noexcept.
• Fix message.

[curdeius]

• Fix typo in comment.

[ldionne]

Except for the nitpick, LGTM.

libcxx/include/array

501

I'll be very nit-picky and ask you to not use auto -> T return types, because we don't use it much elsewhere. I know it's valid, I know it's better style when used throughout a codebase, but I think consistency wins here.

[curdeius]

• Make return type consistent.

[curdeius]

libcxx/include/array

501

Done.

[curdeius]

Just as a side-note, @ldionne, I have commit access, so I can land the patches, but do as you prefer (when the patch gets accepted) :).

[curdeius]

What's the usual way of proceeding, should I land the patch or wait for Louis' approval?

[mclow.lists]

In D69882#1749338, @curdeius wrote:

What's the usual way of proceeding, should I land the patch or wait for Louis' approval?

Wait for Louis (or myself, or Eric). Thanks!

[curdeius]

Ping. :)

[ldionne]

Sorry, this got lost in my inbox. Go ahead and commit it!

[davezarzycki]

This breaks the test suite when built "Release" without assertions enabled (Fedora 31 on x86_64). Can we please revert this?
``
FAIL: libc++ :: std/con:q:qtainers/sequences/array/array.creation/to_array.fail.cpp (53333 of 61086)

• TEST 'libc++ :: std/containers/sequences/array/array.creation/to_array.fail.cpp' FAILED ****

Command: ['/usr/bin/clang++', '-o', '/dev/null', '-x', 'c++', '/home/dave/s/lp/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp', '-c', '-v', '-ftemplate-depth=270', '-fsyntax-only', '-Xclang', '-verify', '-Xclang', '-verify-ignore-unexpected=note', '-ferror-limit=1024', '-Werror=thread-safety', '-std=c++2a', '-include', '/home/dave/s/lp/libcxx/test/support/nasty_macros.h', '-nostdinc++', '-I/home/dave/s/lp/libcxx/include', '-I/tmp/_update_lc/r/projects/libcxx/include/c++build', '-DSTDC_FORMAT_MACROS', '-DSTDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/home/dave/s/lp/libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/home/dave/s/lp/libcxx/test/std/input.output/filesystems/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/tmp/_update_lc/r/projects/libcxx/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /home/dave/s/lp/libcxx/test/support/filesystem_dynamic_test_helper.py"', '-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER', '-Wall', '-Wextra', '-Werror', '-Wuser-defined-warnings', '-Wshadow', '-Wno-unused-command-line-argument', '-Wno-attributes', '-Wno-pessimizing-move', '-Wno-c++11-extensions', '-Wno-user-defined-literals', '-Wno-noexcept-type', '-Wsign-compare', '-Wunused-variable', '-Wunused-parameter', '-Wunreachable-code', '-Wno-error=user-defined-warnings', '-c']
Exit Code: 1

Standard Error:

clang version 9.0.0 (Fedora 9.0.0-1.fc31)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/9
Found candidate GCC installation: /usr/lib/gcc/x86_64-redhat-linux/9
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/9
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64
"/usr/bin/clang-9" -cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -disable-free -disable-llvm-verifier -discard-value-names -main-file-name to_array.fail.cpp -mrelocation-model static -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb -v -nostdinc++ -resource-dir /usr/lib64/clang/9.0.0 -include /home/dave/s/lp/libcxx/test/support/nasty_macros.h -I /home/dave/s/lp/libcxx/include -I /tmp/_update_lc/r/projects/libcxx/include/c++build -D STDC_FORMAT_MACROS -D STDC_LIMIT_MACROS -D __STDC_CONSTANT_MACROS -I /home/dave/s/lp/libcxx/test/support -D "LIBCXX_FILESYSTEM_STATIC_TEST_ROOT=\"/home/dave/s/lp/libcxx/test/std/input.output/filesystems/Inputs/static_test_env\"" -D "LIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT=\"/tmp/_update_lc/r/projects/libcxx/test/filesystem/Output/dynamic_env\"" -D "LIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER=\"/usr/bin/python /home/dave/s/lp/libcxx/test/support/filesystem_dynamic_test_helper.py\"" -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -internal-isystem /usr/local/include -internal-isystem /usr/lib64/clang/9.0.0/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -Werror=thread-safety -Wall -Wextra -Werror -Wuser-defined-warnings -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-error=user-defined-warnings -std=c++2a -fdeprecated-macro -fdebug-compilation-dir /tmp/_update_lc/r -ftemplate-depth 270 -ferror-limit 1024 -fmessage-length 0 -fno-implicit-modules -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -verify -verify-ignore-unexpected=note -faddrsig -x c++ /home/dave/s/lp/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp
clang -cc1 version 9.0.0 based upon LLVM 9.0.0 default target x86_64-unknown-linux-gnu
ignoring nonexistent directory "/include"
#include "..." search starts here:
#include <...> search starts here:
/home/dave/s/lp/libcxx/include
/tmp/_update_lc/r/projects/libcxx/include/c++build
/home/dave/s/lp/libcxx/test/support
/usr/local/include
/usr/lib64/clang/9.0.0/include
/usr/include
End of search list.
error: 'error' diagnostics expected but not seen:

File /home/dave/s/lp/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp Line 20: here
File /home/dave/s/lp/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp Line 25: here
File /home/dave/s/lp/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp Line 30: here

error: 'error' diagnostics seen but not expected:

File /home/dave/s/lp/libcxx/include/array Line 499: static_assert failed due to requirement '!is_array_v<char [6]>' "[array.creation]/1: to_array does not accept multidimensional arrays."
File /home/dave/s/lp/libcxx/include/array Line 502: static_assert failed due to requirement 'is_constructible_v<char [6], char (&)[6]>' "[array.creation]/1: to_array requires copy constructible elements."
File /home/dave/s/lp/libcxx/include/array Line 487: cannot initialize an array element of type 'char' with an lvalue of type 'char [6]'
File /home/dave/s/lp/libcxx/include/array Line 487: cannot initialize an array element of type 'char' with an lvalue of type 'char [6]'
File /home/dave/s/lp/libcxx/include/array Line 487: cannot initialize an array element of type 'char' with an lvalue of type 'char [6]'
File /home/dave/s/lp/libcxx/include/array Line 487: suggest braces around initialization of subobject
File /home/dave/s/lp/libcxx/include/array Line 502: static_assert failed due to requirement 'is_constructible_v<MoveOnly, MoveOnly &>' "[array.creation]/1: to_array requires copy constructible elements."
File /home/dave/s/lp/libcxx/include/array Line 487: calling a private constructor of class 'MoveOnly'
File /home/dave/s/lp/libcxx/include/array Line 514: static_assert failed due to requirement 'is_move_constructible_v<const MoveOnly>' "[array.creation]/4: to_array requires move constructible elements."
Line 493: calling a private constructor of class 'MoveOnly'

13 errors generated.

Expected compilation using verify to pass!

---

Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

10 warning(s) in tests

Testing Time: 140.79s

---

Failing Tests (1):

  libc++ :: std/containers/sequences/array/array.creation/to_array.fail.cpp

Expected Passes    : 44195
Expected Failures  : 99
Unsupported Tests  : 16791
Unexpected Failures: 1

FAILED: CMakeFiles/check-all

[curdeius]

Reverted.

[curdeius]

I've fixed the expected error mesages and relanded it in commit 5e7017273f40a48453ebb758e54123e31f751806 (https://reviews.llvm.org/rGe93e58c6c40a365bc9275dd8a8242598343cc835).

libcxx/include/array

get(const array<_Tp, _Size>&& __a) _NOEXCEPT
     return _VSTD::move(__a.__elems_[_Ip]);
 }
 
 #endif  // !_LIBCPP_CXX03_LANG
 
 #if _LIBCPP_STD_VER > 17
 
 template <typename _Tp, size_t _Size, size_t... _Index>
-_LIBCPP_INLINE_VISIBILITY constexpr auto __to_array(_Tp (&__arr)[_Size],
-                                                    index_sequence<_Index...>)
+_LIBCPP_INLINE_VISIBILITY constexpr auto
+__to_array_lvalue_impl(_Tp (&__arr)[_Size], index_sequence<_Index...>)
     -> array<remove_cv_t<_Tp>, _Size> {
   return {{__arr[_Index]...}};
 }
 
 template <typename _Tp, size_t _Size, size_t... _Index>
-_LIBCPP_INLINE_VISIBILITY constexpr auto __to_array(_Tp(&&__arr)[_Size],
-                                                    index_sequence<_Index...>)
+_LIBCPP_INLINE_VISIBILITY constexpr auto
+__to_array_rvalue_impl(_Tp(&&__arr)[_Size], index_sequence<_Index...>)
     -> array<remove_cv_t<_Tp>, _Size> {
-  return {{move(__arr[_Index])...}};
+  return {{_VSTD::move(__arr[_Index])...}};
 }
 
 template <typename _Tp, size_t _Size>
-_LIBCPP_INLINE_VISIBILITY constexpr auto to_array(_Tp (&__arr)[_Size]) noexcept(
-    is_nothrow_constructible_v<
-        _Tp, _Tp&>) // FIXME: Standard does not say anything about noexcept.
-{
-  static_assert(!is_array_v<_Tp>);
-  static_assert(is_constructible_v<_Tp, _Tp&>);
-  return __to_array(__arr, make_index_sequence<_Size>());
+_LIBCPP_INLINE_VISIBILITY constexpr auto to_array(_Tp (&__arr)[_Size])
+    -> array<remove_cv_t<_Tp>, _Size> {
+  static_assert(
+      !is_array_v<_Tp>,
+      "[array.creation]/1: to_array does not accept multidimensional arrays.");
+  static_assert(
+      is_constructible_v<_Tp, _Tp&>,
+      "[array.creation]/1: to_array requires copy constructible elements.");
+  return __to_array_lvalue_impl(__arr, make_index_sequence<_Size>());
 }
 
 template <typename _Tp, size_t _Size>
 _LIBCPP_INLINE_VISIBILITY constexpr auto to_array(_Tp(&&__arr)[_Size])
-    noexcept(is_nothrow_move_constructible_v<
-             _Tp>) // FIXME: Standard does not say anything about noexcept.
-{
-  static_assert(!is_array_v<_Tp>);
-  static_assert(is_move_constructible_v<_Tp>);
-  return __to_array(std::move(__arr), make_index_sequence<_Size>());
+    -> array<remove_cv_t<_Tp>, _Size> {
+  static_assert(
+      !is_array_v<_Tp>,
+      "[array.creation]/4: to_array does not accept multidimensional arrays.");
+  static_assert(
+      is_move_constructible_v<_Tp>,
+      "[array.creation]/4: to_array requires move constructible elements.");
+  return __to_array_rvalue_impl(_VSTD::move(__arr),
+                                make_index_sequence<_Size>());
 }
 
 #endif // _LIBCPP_STD_VER > 17
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_ARRAY

libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <array>
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 
 #include <array>
-#include <cassert>
-#include <cstddef>
 
 #include "test_macros.h"
+#include "MoveOnly.h"
 
 int main(int, char**) {
   {
     char source[3][6] = {"hi", "world"};
     std::to_array(source); // expected-error {{here}}
   }
 
   {
-    struct move_only {
-      constexpr move_only(int i = 0) : i(i) {}
-      constexpr move_only(move_only&& m) : i(m.i + 1) {}
-      int i;
-    };
-
-    move_only mo;
+    MoveOnly mo[] = {MoveOnly{3}};
     std::to_array(mo); // expected-error {{no matching function}}
+  }
 
-    const move_only cmo;
+  {
+    const MoveOnly cmo[] = {MoveOnly{3}};
     std::to_array(std::move(cmo)); // expected-error {{no matching function}}
   }
 
   return 0;
 }

libcxx/test/std/containers/sequences/array/array.creation/to_array.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <array>
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 
 #include <array>
 #include <cassert>
-#include <cstddef>
 
 #include "test_macros.h"
+#include "MoveOnly.h"
 
 int main(int, char**) {
   //  Test deduced type.
   {
     auto arr = std::to_array({1, 2, 3});
-    static_assert(std::is_same_v<decltype(arr), std::array<int, 3> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<int, 3>);
     assert(arr[0] == 1);
     assert(arr[1] == 2);
     assert(arr[2] == 3);
   }
 
   {
     const long l1 = 42;
     auto arr = std::to_array({1L, 4L, 9L, l1});
-    static_assert(std::is_same_v<decltype(arr)::value_type, long>, "");
+    ASSERT_SAME_TYPE(decltype(arr)::value_type, long);
     static_assert(arr.size() == 4, "");
     assert(arr[0] == 1);
     assert(arr[1] == 4);
     assert(arr[2] == 9);
     assert(arr[3] == l1);
   }
 
   {
     auto arr = std::to_array("meow");
-    static_assert(std::is_same_v<decltype(arr), std::array<char, 5> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<char, 5>);
     assert(arr[0] == 'm');
     assert(arr[1] == 'e');
     assert(arr[2] == 'o');
     assert(arr[3] == 'w');
     assert(arr[4] == '\0');
   }
 
   {
     double source[3] = {4.0, 5.0, 6.0};
     auto arr = std::to_array(source);
-    static_assert(std::is_same_v<decltype(arr), std::array<double, 3> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<double, 3>);
     assert(arr[0] == 4.0);
     assert(arr[1] == 5.0);
     assert(arr[2] == 6.0);
   }
 
   {
     double source[3] = {4.0, 5.0, 6.0};
     auto arr = std::to_array(std::move(source));
-    static_assert(std::is_same_v<decltype(arr), std::array<double, 3> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<double, 3>);
     assert(arr[0] == 4.0);
     assert(arr[1] == 5.0);
     assert(arr[2] == 6.0);
   }
 
   {
-    struct move_only {
-      constexpr move_only(int i = 0) : i(i) {}
-      constexpr move_only(move_only&& m) : i(m.i + 1) {}
-      int i;
-    };
-    struct some_move_only {
-      move_only m[3];
-    };
+    MoveOnly source[] = {MoveOnly{0}, MoveOnly{1}, MoveOnly{2}};
 
-    auto arr = std::to_array(some_move_only{{0, 1, 2}}.m);
-    static_assert(std::is_same_v<decltype(arr), std::array<move_only, 3> >, "");
-    assert(arr[0].i == 1);
-    assert(arr[1].i == 2);
-    assert(arr[2].i == 3);
+    auto arr = std::to_array(std::move(source));
+    ASSERT_SAME_TYPE(decltype(arr), std::array<MoveOnly, 3>);
+    for (int i = 0; i < 3; ++i)
+      assert(arr[i].get() == i && source[i].get() == 0);
   }
 
   // Test C99 compound literal.
   {
     auto arr = std::to_array((int[]){3, 4});
-    static_assert(std::is_same_v<decltype(arr), std::array<int, 2> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<int, 2>);
     assert(arr[0] == 3);
     assert(arr[1] == 4);
   }
 
   //  Test explicit type.
   {
     auto arr = std::to_array<long>({1, 2, 3});
-    static_assert(std::is_same_v<decltype(arr), std::array<long, 3> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<long, 3>);
     assert(arr[0] == 1);
     assert(arr[1] == 2);
     assert(arr[2] == 3);
   }
 
   {
     struct A {
       int a;
       double b;
     };
 
     auto arr = std::to_array<A>({{3, .1}});
-    static_assert(std::is_same_v<decltype(arr), std::array<A, 1> >, "");
+    ASSERT_SAME_TYPE(decltype(arr), std::array<A, 1>);
     assert(arr[0].a == 3);
     assert(arr[0].b == .1);
   }
 
+  // Test constexpr.
+  {
+    constexpr std::array<int, 3> arr = std::to_array({1, 2, 3});
+    static_assert(arr[0] == 1);
+    static_assert(arr[1] == 2);
+    static_assert(arr[2] == 3);
+  }
+
   return 0;
 }