Page MenuHomePhabricator

Update shared_ptr tests to match the standard
Needs ReviewPublic

Authored by zoecarver on May 18 2019, 1:28 PM.

Details

Summary

This patch updates the shared_ptr tests so that they more closely and more completely match the standard. I think it is essential that these tests match exactly what the standard says and test it completely because I am going to use them when making significant changes to shared_ptr.

For reference here is the relevant section of the current standard.

Currently, I have only updated the constructor tests. However, I plan on updating all of the shared_ptr tests (either in this patch or another).

Diff Detail

Event Timeline

zoecarver created this revision.May 18 2019, 1:28 PM

There's a lot of whitespace changes here. And they hide what's actually changed. Would it be possible to generate a version of this patch without all the noise?

  • white space
  • a few test issues

Zoe - you're not running these tests sufficiently.
They fail under c++03, c++11 and c++14

  • min_allocator is a c++11-specific test tool.
  • void_t is not available until C++17
  • static_assert w/o a message is a C++17 thing.
mclow.lists added inline comments.May 23 2019, 8:29 AM
test/std/utilities/memory/util.smartptr/util.smartptr.shared/types.pass.cpp
45

Two points here.

In C++03, we have an emulation of static_assert that is a macro. Sadly, like all macros, it gets confused by commas. "Is this a comma in the expression, or another macro parameter?" Hence, the extra parenthesis which you removed. Please put them back.

Also, why not use ASSERT_SAME_TYPE?

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/default.pass.cpp
27

Why braces here?

You might want to read up on the difference between default-initialization and value-initialization.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer.pass.cpp
37 ↗(On Diff #200792)

Is "whitespace" the only change to this file?

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
39 ↗(On Diff #200792)

By removing this space, you make it so it doesn't work in C++03.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp
72 ↗(On Diff #200792)

min_allocator is a C++11 test thing.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_Y_rv.pass.cpp
72 ↗(On Diff #200792)

I don't see any explanation as to why you're stripping out these C++03 tests.

96 ↗(On Diff #200792)

I would think that this test should go on line 87

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_copy_move.fail.cpp
43

Why is this bit C++14 only?

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_pointer.pass.cpp
75

Again, why 14?

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp
10

Why disable this entire test for C++98/03?

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
83 ↗(On Diff #200792)

This comment is confusing to me.
It checks for +1, but the comment implies two.

(There should be only one allocation, btw).

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.obs/op_bool.pass.cpp
41

This seems convoluted. Why not:

std::shared_ptr<B> sp = std::dynamic_pointer_cast<B>(basePtr);
assert(sp);
mclow.lists requested changes to this revision.May 23 2019, 8:56 AM

I should have flagged this before.

This revision now requires changes to proceed.May 23 2019, 8:56 AM
zoecarver marked 6 inline comments as done.May 23 2019, 9:48 AM

This patch is not the quality I would have liked. When I rolled back some of the changes I made, it left only whitespace changes which were not intentional. I also should have run the tests for 11, 14, and 17. When I updated this patch, I did not understand what versions of C++ shared_ptr was supported in. That is why I removed many of the C++11 checks and support for C++03 test cases. I will fix the issues around versioning and white space.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/types.pass.cpp
45

Will do.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/default.pass.cpp
27

This is to test that both work.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
39 ↗(On Diff #200792)

Because that would be ambiguous. Ugh, you are right. Will fix.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_copy_move.fail.cpp
43

Only since C++17 was this added:

This overload doesn't participate in overload resolution if std::unique_ptr<Y, Deleter>::pointer is not compatible with T*

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
83 ↗(On Diff #200792)

Oops, meant to get rid of this.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.obs/op_bool.pass.cpp
41

Yes, that would be simpler. I will update.

I originally wrote it in that way to match the note in the standard.

zoecarver marked 2 inline comments as done.
  • remove spacing only changes
  • fix versioning
  • fix based on review
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_Y_rv.pass.cpp
96 ↗(On Diff #200792)

The point is to assert both pA and pB point to nullptr. I can update.

test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_pointer.pass.cpp
75

This should be after C++17. This is testing the second overload for rvalue references: http://eel.is/c++draft/util.smartptr.shared#const-15

zoecarver marked an inline comment as done.Jun 6 2019, 6:33 PM
zoecarver added inline comments.
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/default.pass.cpp
27

This is the default.pass.cpp file. I see. Will remove.

zoecarver updated this revision to Diff 203475.Jun 6 2019, 6:35 PM
  • update tests based on the review
  • general test fixes