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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/array | ||
---|---|---|
501 | I don't think you need the noexcept. | |
libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp | ||
15 | Is this needed? | |
26 | You can get this with #include "MoveOnly.h". | |
libcxx/test/std/containers/sequences/array/array.creation/to_array.pass.cpp | ||
23 | You can use ASSERT_SAME_TYPE (from test_macros). |
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).
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.
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).
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?
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:
It seems like they should have said this instead?
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 | ||
11 | Can you please add the synopsis of the exact functions you're testing (so the two overloads of to_array)? |
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 | ||
26 | 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 :) |
Adressed comments.
libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp | ||
---|---|---|
26 | Oops, fixed it. |
LGTM.
IIRC implementations can make functions "more" noexcept. If so then noexcept is welcome.
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. |
libcxx/include/array | ||
---|---|---|
501 | Done. |
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) :).
What's the usual way of proceeding, should I land the patch or wait for Louis' approval?
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
I've fixed the expected error mesages and relanded it in commit 5e7017273f40a48453ebb758e54123e31f751806 (https://reviews.llvm.org/rGe93e58c6c40a365bc9275dd8a8242598343cc835).
You can use constexpr directly here (and elsewhere), since this is only enabled when _LIBCPP_STD_VER > 17.