Page MenuHomePhabricator

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

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
EricWF added inline comments.Aug 30 2015, 3:17 PM
include/experimental/propagate_const
11 ↗(On Diff #33549)

_LIBCPP_EXPERIMENTAL_PROPAGATE_CONST please.

126 ↗(On Diff #33549)

_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
24 ↗(On Diff #34364)

s/__propagate/propagate

112 ↗(On Diff #34364)

You'll want to add this below the include:

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif
154 ↗(On Diff #34364)

Use a reserved name. ie __is_propagate_const.

170 ↗(On Diff #34364)

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

174 ↗(On Diff #34364)

Same as above.

176 ↗(On Diff #34364)

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.

208 ↗(On Diff #34364)

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
14 ↗(On Diff #37711)

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
26 ↗(On Diff #37711)

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
36 ↗(On Diff #37711)

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
29 ↗(On Diff #37711)

"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
27 ↗(On Diff #37711)

"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
22 ↗(On Diff #37711)

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
14 ↗(On Diff #37711)

On reflection it looks ok.

test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.ctors/element_type.non-explicit.ctor.pass.cpp
29 ↗(On Diff #37711)

Removed spurious comment.

test/std/experimental/utilities/propagate_const/propagate_const.nonmembers/propagate_const.comparison_function_objects/less_equal.pass.cpp
23 ↗(On Diff #55188)

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
128 ↗(On Diff #37711)

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
275 ↗(On Diff #55188)

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);
}
284 ↗(On Diff #55188)

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
129 ↗(On Diff #59775)

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 ↗(On Diff #59789)

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.