Page MenuHomePhabricator

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

Authored by curdeius on Nov 6 2019, 1:32 AM.

Details

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.

Diff Detail

Event Timeline

curdeius created this revision.Nov 6 2019, 1:32 AM
Herald added a project: Restricted Project. · View Herald Transcript
curdeius retitled this revision from [libcxx] Add P0325 - to_array. to [libcxx] Implement P0325: to_array from LFTS with updates..Nov 6 2019, 1:40 AM
curdeius edited the summary of this revision. (Show Details)

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

curdeius updated this revision to Diff 229025.Nov 13 2019, 1:25 AM
  • Clean up to_array implementation.
  • Add tests from D66103. Clean up.
curdeius updated this revision to Diff 229026.Nov 13 2019, 1:32 AM
  • Fix typos.
curdeius marked 4 inline comments as done.Nov 13 2019, 1:36 AM

Updated with tests from D66103. Answered issues from comments.

Sorry for the delay.

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 requested changes to this revision.Nov 13 2019, 8:03 AM
ldionne added a subscriber: lichray.
ldionne added inline comments.
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
11

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

This revision now requires changes to proceed.Nov 13 2019, 8:03 AM
lichray added inline comments.Nov 13 2019, 8:32 AM
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 :)

ldionne added inline comments.Nov 13 2019, 9:31 AM
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 updated this revision to Diff 229132.Nov 13 2019, 10:08 AM
  • Fix constexpr and return type.
  • Fix tests.
curdeius updated this revision to Diff 229134.Nov 13 2019, 10:11 AM
  • Add synopsis.
Harbormaster completed remote builds in B40902: Diff 229134.
curdeius marked 8 inline comments as done.Nov 13 2019, 12:27 PM

Adressed comments.

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

Oops, fixed it.

curdeius updated this revision to Diff 229164.Nov 13 2019, 12:50 PM
curdeius marked an inline comment as done.

Fix patch diff.

LGTM.

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

curdeius updated this revision to Diff 229171.Nov 13 2019, 1:19 PM
  • Readd noexcept.
  • Fix message.
curdeius updated this revision to Diff 229174.Nov 13 2019, 1:22 PM
  • Fix typo in comment.
curdeius retitled this revision from [libcxx] Implement P0325: to_array from LFTS with updates. to [libc++] [P0325] Implement to_array from LFTS with updates..Nov 13 2019, 10:47 PM
ldionne requested changes to this revision.Nov 14 2019, 5:58 AM

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.

This revision now requires changes to proceed.Nov 14 2019, 5:58 AM
curdeius updated this revision to Diff 229393.Nov 14 2019, 1:27 PM
  • Make return type consistent.
curdeius marked 2 inline comments as done.Nov 14 2019, 1:28 PM
curdeius added inline comments.
libcxx/include/array
501

Done.

curdeius marked an inline comment as done.Nov 14 2019, 2:09 PM

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

lichray accepted this revision.Nov 14 2019, 2:18 PM

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

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!

ldionne accepted this revision.Jan 29 2020, 5:02 PM

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

This revision is now accepted and ready to land.Jan 29 2020, 5:02 PM
This revision was automatically updated to reflect the committed changes.

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