This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add <experimental/any> v2.
ClosedPublic

Authored by EricWF on Dec 22 2014, 3:15 PM.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 17574.Dec 22 2014, 3:15 PM
EricWF retitled this revision from to [libcxx] Add <experimental/any> v2..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added a subscriber: Unknown Object (MLST).
EricWF updated this revision to Diff 19155.Feb 2 2015, 7:57 AM

To address @mclow.lists initial comments I've copied most of the tests in std/experimental/any to libcxx/experimental/any. I have also changed the tests in std/experimental/any to no longer check anything that tests non-standard behaviour. The tests under std/experimental/any pass against libstdc++'s implementation as well.

There is a lot of test duplication between std/experimental/any and libcxx/experimental/any. I'm going to work on reducing this by removing or trimming the tests in libcxx/experimental/any. However the tests in std should be complete and can be reviewed.

mclow.lists edited edge metadata.Mar 16 2015, 1:01 PM

Reviewed section any.cons

test/std/experimental/any/any.class/any.cons/copy.pass.cpp
35

I guess I'm OK with using large and small here (instead of int and complex<long double>, say), but I think that would be better.

If you're going to explicitly check small object optimizations, that should not be in the "test/std/" hierarchy.

test/std/experimental/any/any.class/any.cons/copy_throws.pass.cpp
24 ↗(On Diff #19155)

Again; large and small.

test/std/experimental/any/any.class/any.cons/default.pass.cpp
36

Do you want to assert this before the any is created as well?

test/std/experimental/any/any.class/any.cons/move.pass.cpp
43

same comment as for copy-constructor for large vs. small

test/std/experimental/any/any.class/any.cons/move_throws.pass.cpp
39 ↗(On Diff #19155)

Looks good.

test/std/experimental/any/any.class/any.cons/non_copyable_value.fail.cpp
27

Shouldn't this really be non_copyable & non_copyable(non_copyable &&) { return *this; } ??

and similar for the declaration of the copy constructor.

I find it hard to write good "compile failing" tests. Frequently they fail for the wrong reasons.

test/std/experimental/any/any.class/any.cons/value.pass.cpp
56

Not std::move(s) ?

99

See line #55

test/std/experimental/any/any.class/any.cons/value_copy_throws.pass.cpp
53 ↗(On Diff #19155)

Looks good.

test/std/experimental/any/any.class/any.cons/value_move_throws.pass.cpp
26 ↗(On Diff #19155)

You have a small_throws_on_copy and a large_throws_on_copy; but only a single throws_on_move.

Why the difference?

Address @mclow.lists comments.

test/std/experimental/any/any.class/any.cons/copy.pass.cpp
35

All of the tests in the test/std hierarchy should pass regardless of whether the implementation provides the small object optimization. I test both cases inside test/std because if I didn't the tests here would only provides about ~50% coverage. Any test cases that actually

There are two reasons I prefer using the names large and small (even if they alias int and complex<long double>).

  1. If the conditions for the SOO change then it is easy to change either large or small to match.
  2. It's more clear why there is duplication in the tests for the two different types.

The reason I like actually using the large and small classes is that they provide a little extra support for testing things like construction, copying and moving.

test/std/experimental/any/any.class/any.cons/default.pass.cpp
36

Probably. Thanks.

test/std/experimental/any/any.class/any.cons/non_copyable_value.fail.cpp
27

I don't think so... Could you explain?

test/std/experimental/any/any.class/any.cons/value.pass.cpp
56

Fixed.

99

Fixed.

test/std/experimental/any/any.class/any.cons/value_move_throws.pass.cpp
26 ↗(On Diff #19155)

Any type that throws on move construction cannot have the SOO applied to it.

EricWF updated this revision to Diff 22096.Mar 17 2015, 8:45 AM
EricWF edited edge metadata.

Fix review comments.

EricWF updated this revision to Diff 30200.Jul 20 2015, 3:25 PM

Reworked the entire test suite. It should be cleaner and easier to review now.

One issue and a few nits.

src/any.cpp
19

Because of arcane packaging issues, we put the exception classes in an unversioned namespace and the destructors in the the dylib as well. See <optional> implementation.

_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL
const char* bad_any_cast::what() const _NOEXCEPT
{
    return "bad any cast";
}

bad_any_cast::~ bad_any_cast() _NOEXCEPT {}
_LIBCPP_END_NAMESPACE_EXPERIMENTAL

This will require a change in the header file as well

test/std/experimental/any/any.class/any.assign/copy.pass.cpp
198

Should probably fix this. (add newline)

test/std/experimental/any/any.class/any.assign/move.pass.cpp
103

and this (newline)

test/std/experimental/any/any.class/any.modifiers/clear.pass.cpp
43

I think that adding an assertNotEmpty<small>(a) here (and on line #52) will make the test better.

For completeness, you could add assertEmpty(a) on line #34 as well.

mclow.lists accepted this revision.Jul 22 2015, 8:35 AM
mclow.lists edited edge metadata.

With the fixes that I've noted, this LGTM.

This revision is now accepted and ready to land.Jul 22 2015, 8:35 AM
EricWF marked 2 inline comments as done.Jul 22 2015, 2:36 PM

@mclow.lists: I addressed your comment about the exception namespace. I'll wait for your response before moving forward.

src/any.cpp
19

I don't think this makes sense for things in the LFTS. Unlike std::__1 which is an implementation detail, the LFTS explicitly puts the exceptions in the inline namespace and putting bad_any_cast in std::experimental is non-conforming.

AFAIK there are two reasons we generally put exceptions in namespace std directly and not std::__1

  1. ABI stability. When std::__1 changes to std::__2 the exception types are still compatible.
  2. libstdc++ compatibility. libc++'s exceptions should be ABI compatible with GCC's when possible.

ABI stability doesn't really make sense when thinking about TS's because they are not meant to be stable.

Also If we put bad_any_cast outside of fundamentals_v1 we lose exception compatibility with libstdc++.

@mclow.lists: Do you still want to make this change?

EricWF added inline comments.Jul 22 2015, 2:42 PM
src/any.cpp
19

libstdc++ actually puts bad_any_cast in std::experimental::fundamentals_v1::__7 so we aren't going to get libstdc++ compatibility regardless.

mclow.lists added inline comments.Jul 22 2015, 2:57 PM
src/any.cpp
19

so libstdc++ compatibility doesn't matter.

I guess I'm mildly in favor of putting it into namespace std::experimental - but it's not a deal-breaker for me. I'm more interested in consistency; we should make sure that bad_optional_access and bad_any_cast are in the same namespace.

EricWF updated this revision to Diff 31101.Jul 30 2015, 6:52 PM
EricWF edited edge metadata.

Fix some final issues:

  • Get tests passing with -fno-rtti and -fno-exceptions
  • Prevent vtable of bad_any_cast from being emitted by placing constructor and destructor definitions in the dylib.
  • Remove duplication of _LIBCPP_INLINE_VISIBILITY _LIBCPP_ALWAYS_INLINE
  • Fix bad clear test.
EricWF closed this revision.Jul 30 2015, 7:25 PM