This patch adds the second revision of <experimental/any>.
I've been working from the LFTS draft found at this link. https://rawgit.com/cplusplus/fundamentals-ts/v1/fundamentals-ts.html#any
Details
Diff Detail
Event Timeline
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.
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>).
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. |
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: 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
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? |
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. |
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. |
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.
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.
This will require a change in the header file as well