This is an archive of the discontinued LLVM Phabricator instance.

constexpr array support C++1z (P0031)
Needs ReviewPublic

Authored by lefticus on Jul 20 2016, 12:57 PM.

Details

Reviewers
mclow.lists
Summary
  • array constexpr-ized
  • associated iterator functions constexpr-ized
  • unit tests added for array
  • unit tests added for iterator

Unit tests for std::array required non-const constexpr context for tests, so constexpr helper functions were added in unit tests, with static_assert calls on resulting values.

Ref: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0031r0.html

Diff Detail

Event Timeline

lefticus updated this revision to Diff 64731.Jul 20 2016, 12:57 PM
lefticus retitled this revision from to C++1z constexpr support for array and iterator.
lefticus updated this object.
lefticus added reviewers: mclow.lists, EricWF.
mclow.lists edited edge metadata.Jul 20 2016, 1:10 PM

All look fine to me - but needs tests.

lefticus edited edge metadata.Jul 20 2016, 1:13 PM
lefticus added a subscriber: cfe-commits.
lefticus updated this revision to Diff 65143.Jul 22 2016, 1:55 PM
lefticus retitled this revision from C++1z constexpr support for array and iterator to constexpr array support C++1z (P0031).
lefticus updated this object.
  • Updated title and summary to be more accurate.
  • Implemented tests for constexpr std::array access.
  • Added required constexpr iterator support.
  • Implemented tests for constexpr iterator functions
EricWF edited edge metadata.Jul 24 2016, 4:06 PM

For the most part this LGTM. Thanks for putting in all this work. I've noted some places where the test coverage could be improved.

include/array
156

This is untested.

160

This is untested.

165

Untested.

169

Untested.

208

Untested.

include/iterator
527

Untested.

536

Untested.

565

Untested.

1132

This is a C++03 only branch, but I guess it's more consistent to apply the macro.

test/std/containers/sequences/array/indexing.pass.cpp
41

Missing conditional directive terminator.

86

C doesn't name a type.

test/std/containers/sequences/array/iterators.pass.cpp
113

Include test_macros.h and use TEST_STD_VER please.

lefticus updated this revision to Diff 65390.Jul 25 2016, 11:30 AM
lefticus edited edge metadata.

Fixed syntax errors in test files.

lefticus marked 3 inline comments as done.Jul 25 2016, 11:51 AM

I believe I messed up the context for the files, will attempt to resubmit the patch.

lefticus updated this revision to Diff 65398.Jul 25 2016, 12:07 PM

Correct context from last patch

I believe all or most of the "untested" comments are correctly tested in the updated test files via static_asserts with contexpr. I've commented on just the first one to make sure I understand correctly.

include/array
156

This should be tested in begin.pass.cpp:28 correct?

This diff no longer applies cleanly, and I've added some formatting concerns. However, the content looks fine to me.

include/array
176

Nit: I would put the _LIBCPP_CONSTEXPR_AFTER_CXX14 on the same line as _LIBCPP_INLINE_VISIBILITY to keep the lines from getting too long.

190–191

And break this line after the macros for the same reason as L#176

include/iterator
526

Here, I would smoosh them all together on the same line: inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14

lefticus updated this revision to Diff 69195.Aug 24 2016, 7:59 PM

Address formatting concerns and out-of-date-ness of patch relative to trunk/master

Other than the iterator.primitives/iterator.operations/prev.pass.cpp test, I think this is good to go.

test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp
47

Won't this fail on C++11 (i.e, doesn't it need a #if TEST_STD_VER > 14)?

lefticus updated this revision to Diff 71668.Sep 16 2016, 9:49 AM

Add C++1z check around constexpr tests, based on mclow's feedback

@lefticus Are you still interested in working on this? If so could you please split the changes for array into a separate patch? Marshall seems to have implemented some of the changes in <iterator> and that's causing merge conflicts.

lefticus marked an inline comment as done.EditedDec 30 2016, 6:49 AM
lefticus added a subscriber: EricFowler.

@EricWF I have too many other things going on right now to be able to look at it for at least the next few months.

EricWF resigned from this revision.Jan 4 2017, 12:30 PM
EricWF removed a reviewer: EricWF.

Looks like @mclow.lists just committed another version of this paper, so this patch is no longer needed.

Resigning as reviewer.