This is an archive of the discontinued LLVM Phabricator instance.

std::experimental::propagate_const from LFTS v2
ClosedPublic

Authored by jbcoe on Aug 30 2015, 3:01 PM.

Details

Summary

An implementation of std::experimental::propagate_const from Library Fundamentals Technical Specification v2.

Tests are provided for specified behaviour (including non-compiling code by checking type_traits).

No tests are provided for disallowed types like fancy pointers or function pointers as no code was written to handle these.

Diff Detail

Event Timeline

jbcoe updated this revision to Diff 33549.Aug 30 2015, 3:01 PM
jbcoe retitled this revision from to std::experimental::propagate_const from LFTS v2.
jbcoe updated this object.
jbcoe added reviewers: mclow.lists, EricWF.
EricWF edited edge metadata.Aug 30 2015, 3:17 PM

Awesome and thanks for doing this. I've added some drive-by comments but I haven't really looked at it yet.

I'll start trying to figure out how libc++ should manage shipping both LFTS v1 and LFTS v2 headers.

include/experimental/propagate_const
131

Anything that isn't in the draft should use a reserved name. So this should probably be __get_pointer.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign.fail.cpp
21 ↗(On Diff #33549)

If the test types X and Y are the same in every test then please lift them into a test header.

EricWF added inline comments.Aug 30 2015, 3:17 PM
include/experimental/propagate_const
12

_LIBCPP_EXPERIMENTAL_PROPAGATE_CONST please.

127

_VSTD::declval to prevent ADL.

jbcoe added a comment.Aug 30 2015, 3:23 PM

Is there a good way to see if tests that fail (as expected) are failing for the right reason?

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign.fail.cpp
21 ↗(On Diff #33549)

X is always the same and Y is nearly always the same. I can split Y into Y and Y_explicit.
I assume that the test header would go in the root of the propagate_const tests directory?

Is there a good way to see if tests that fail (as expected) are failing for the right reason?

Yes. We use clang verify (http://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html#details) to check compilation failure tests. If a *.fail.cpp test contains any verify directives (// expected-{note,warning,error}) then that test is run using clang verify automatically.

Take a look at the following tests for examples:

  • std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/default01.fail.cpp
  • std/thread/futures/futures.promise/copy_ctor.fail.cpp

You only have to supply the expected diagnostics for errors and not notes. Let me know if I can make anything more clear.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign.fail.cpp
21 ↗(On Diff #33549)

I would put the header in libcxx/test/support which is an include directory that all tests share.

jbcoe updated this revision to Diff 33635.Aug 31 2015, 2:59 PM
jbcoe edited edge metadata.

Refactor tests to use helper classes.

Add __ to get_underlying.

Use _VSTD:: for declval to prevent ADL-induced name clashes.

jbcoe marked 5 inline comments as done.Aug 31 2015, 3:01 PM
jbcoe updated this revision to Diff 33636.Aug 31 2015, 3:04 PM

Correctly add _VSTD:: to declval in definition of element_type.

jbcoe marked an inline comment as done.Aug 31 2015, 3:05 PM
jbcoe updated this revision to Diff 33745.Sep 1 2015, 3:31 PM

Correct spelling of 'Constructable'

mclow.lists edited edge metadata.Sep 8 2015, 12:07 PM

Correct spelling of 'Constructable'

While I approve of this cleanup, I don't think it belongs here - and makes reviewing the propogate_const stuff harder.

jbcoe added a comment.Sep 9 2015, 1:06 PM

This was utterly unintentional and very embarassing. I only mean to amend the propagate-const files.
I'll revert and update the patch.

jbcoe updated this revision to Diff 34364.Sep 9 2015, 1:13 PM
jbcoe edited edge metadata.

Undo spelling correction as it touched many more files than intended.

EricWF added a comment.Oct 6 2015, 5:12 PM

First I think the header is in great shape. I reviewed the constructors and they look good other than the inline nits. I would like to see some more tests though. If a constructor is constexpr then it should have a constexpr test.

I'll try and go through another section of it later this week.

include/experimental/propagate_const
25

s/__propagate/propagate

113

You'll want to add this below the include:

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif
155

Use a reserved name. ie __is_propagate_const.

171

Just use = default directly. Other C++14 headers already do. Currently this constructor won't compile when _LIBCPP_DEFAULT evaluates to {}

175

Same as above.

177

is_convertible<Tp&&, Up> should be equivalent to is_convertible<Tp, Up>.I don't know why the LFTS draft adds the extra reference qualifiers but I"m 95% sure its redundant because declval<Tp>() should already do this for you.

209

Same as above. Don't use _LIBCPP_DEFAULT here.

jbcoe updated this revision to Diff 37709.Oct 18 2015, 7:00 AM

Address comments from Eric's review. Constexpr tests outstanding (How do I do this?).

jbcoe marked 7 inline comments as done.Oct 18 2015, 7:01 AM
jbcoe updated this revision to Diff 37710.Oct 18 2015, 9:10 AM

constexpr fixes and addition of constexpr to some tests.

I cannot get operator element_type* to work with constexpr - some input here would be good.

jbcoe updated this revision to Diff 37711.Oct 18 2015, 9:14 AM

Use _LIBCXX_CONSTEXPR not constexpr

jbcoe abandoned this revision.Jan 18 2016, 1:34 PM
jbcoe reclaimed this revision.Jan 18 2016, 2:53 PM

Abandoned in error.

Phew! I'll try and look through more of this tomorrow.

jbcoe added inline comments.Mar 4 2016, 6:17 PM
test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign_convertible_propagate_const.fail.cpp
25 ↗(On Diff #37711)

Y is undefined this needs fixing, compiling this file fails for the wrong reason

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign_element_type.pass.cpp
15

This comment is not right.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/convertible_element_type.explicit.ctor.fail.cpp
22 ↗(On Diff #37711)

Y is undefined. compilation fails for the wrong reason.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/convertible_propagate_const.copy_ctor.fail.cpp
22 ↗(On Diff #37711)

Y is undefined. compilation fails for the wrong reason.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/convertible_propagate_const.explicit.move_ctor.fail.cpp
22 ↗(On Diff #37711)

Y is undefined. compilation fails for the wrong reason.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.observers/operator_element_type_ptr.pass.cpp
27

if this is made global then this test will do what's required.

test/std/experimental/utilities/propagate_const/propagate_const.class/swap.pass.cpp
37

a test for non-member swap may be needed (just below)

jbcoe marked an inline comment as done.Mar 5 2016, 3:25 PM
jbcoe planned changes to this revision.Mar 22 2016, 4:18 AM

All the comments seem to be about the tests, which is good (and bad). Good, since the code seems to be pretty solid; bad because it means the tests need more work.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/element_type.non-explicit.ctor.pass.cpp
30

"Will not compile" ?? -- in a test named "xxx.pass.cpp"? Which is correct? The test, the name of the test, or the comment?

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/move_ctor.pass.cpp
28

"Will not compile" ?? -- in a test named "xxx.pass.cpp"? Which is correct? The test, the name of the test, or the comment?

test/std/experimental/utilities/propagate_const/propagate_const.nonmembers/propagate_const.comparison_function_objects/less_equal.pass.cpp
23

Is there some reason that you shouldn't just say:

bool operator<=(const X& lhs, const X& rhs) { return lhs.i <= rhs.i; }

and not open up namespace std?

jbcoe updated this revision to Diff 55188.Apr 27 2016, 4:59 AM
jbcoe marked 9 inline comments as done.

Corrections from Marshall's review.

jbcoe added inline comments.Apr 27 2016, 5:01 AM
test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.assignment/assign_element_type.pass.cpp
15

On reflection it looks ok.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/element_type.non-explicit.ctor.pass.cpp
30

Removed spurious comment.

test/std/experimental/utilities/propagate_const/propagate_const.nonmembers/propagate_const.comparison_function_objects/less_equal.pass.cpp
24

no. I've updated code to be more concise.

jbcoe added a comment.May 12 2016, 1:55 PM

Is more work required on this?

Jonathan - if you're happy with all the tests, especially if the failing tests fail for the right reasons, then I'm happy with you committing it.

I have some feedback before committing. I'll submit it shortly.

include/experimental/propagate_const
129

Please add static asserts before this that check 3.7.3[propagate_const.requirments]. This will lead to much nicer diagnostics.

Many of your failure tests can be rewritten as .pass.cpp tests using type traits. Esspecially the ones for the ctors.

For example convertible_element_type.explicit.ctor.fail.cpp can be written as a single static assertion.

static_assert(!is_convertible<int, propagate_const<ExplicitX>>::value, "");

Other than those changes this LGTM like Marshall said.

include/experimental/propagate_const
276

This call should be unqualified. It can't be implemented directly within the class though. I would do "_VSTD_LFTS_V2::swap(*this, __pt)" here. Please add a test for this of the form:

c++

bool swap_called = false;
void swap(X&, X&) {swap_called = true;}

int main() {
  typedef propagate_const<X> P;
  P p1(1);
  P p2(2);
  p1.swap(p2);
  assert(swap_called);
}
285

All calls to get_underlying should to be qualified with _VSTD_LFTS_V2. Currently all uses of get_underlying generate a perfect match with the two overloads we provide so it won't be hijacked by ADL but that may change.

FYI I generated some code coverage results for this patch: They can be found here http://efcs.ca/const-coverage/include/experimental/propagate_const.gcov.html.

The stats are misleading because uninstantiated templates aren't counted as uncovered.

jbcoe updated this revision to Diff 58217.May 24 2016, 4:10 AM

All tests are now 'pass' tests as static assert can be used to check for invalid type traits for previous failure cases.

jbcoe updated this object.May 24 2016, 4:13 AM

Once the swap issue is resolved I think this is good to go. @jbcoe do you have commit access?

EricWF resigned from this revision.May 31 2016, 12:47 AM
EricWF removed a reviewer: EricWF.
EricWF added a reviewer: EricWF.
EricWF added a subscriber: EricWF.

Woops, I thought this had landed.

jbcoe updated this revision to Diff 59775.Jun 6 2016, 1:27 PM

ADL-focused fixes from Eric's review (thanks for this).

jbcoe marked 4 inline comments as done.Jun 6 2016, 1:30 PM
jbcoe added inline comments.
include/experimental/propagate_const
130

Do you mean in the class body?

jbcoe updated this revision to Diff 59789.Jun 6 2016, 2:40 PM

Add static_asserts to class body for constraints on template type.

jbcoe marked 2 inline comments as done.Jun 6 2016, 2:41 PM
jbcoe added inline comments.
include/experimental/propagate_const
130

I can't see what else you might mean. Perhaps I could add enable_if's but I followed the approach taken in optional.

jbcoe marked an inline comment as done.Jun 8 2016, 1:15 AM
jbcoe added a comment.Jun 17 2016, 3:25 PM

I think I've addressed all comments. Is anything outstanding or is this good to go?

I've got nothing else. LGTM.

mclow.lists accepted this revision.Jun 17 2016, 4:41 PM
mclow.lists edited edge metadata.
This revision is now accepted and ready to land.Jun 17 2016, 4:41 PM
jbcoe updated this revision to Diff 61198.Jun 19 2016, 4:46 AM
jbcoe edited edge metadata.

Resolve conflict with upstream master to allow arc to apply patch.

This revision was automatically updated to reflect the committed changes.