[libcxx-commits] [PATCH] D62103: Update shared_ptr tests to match the standard

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 6 inline comments as done.
zoecarver added a comment.

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.



================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/types.pass.cpp:39
 {
-    static_assert((std::is_same<std::shared_ptr<A>::element_type, A>::value), "");
+    static_assert(std::is_same<typename std::shared_ptr<T>::element_type, T>::value);
 #if TEST_STD_VER > 14
----------------
mclow.lists wrote:
> 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`?
> 
Will do.


================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/default.pass.cpp:27
+    {
+        std::shared_ptr<T> p {};
+        assert(p.use_count() == 0);
----------------
mclow.lists wrote:
> Why braces here? 
> 
> You might want to read up on the difference between default-initialization and value-initialization.
> 
This is to test that both work. 


================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp:39
+
+        test_deleter<A>* d = std::get_deleter<test_deleter<A>>(p);
+        assert(test_deleter<A>::count == 1);
----------------
mclow.lists wrote:
> By removing this space, you make it so it doesn't work in C++03.
Because that would be ambiguous. Ugh, you are right. Will fix. 


================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_copy_move.fail.cpp:42
+
+#if TEST_STD_VER > 14
+    {
----------------
mclow.lists wrote:
> Why is this bit C++14 only?
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*


================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp:83
     std::shared_ptr<A> p = std::make_shared<A>(i, c);
-    assert(globalMemCounter.checkOutstandingNewEq(nc+1));
+    assert(globalMemCounter.checkOutstandingNewEq(nc+1)); // one for make_shared (create type ptr) and one for creation of control block.
     assert(A::count == 1);
----------------
mclow.lists wrote:
> This comment is confusing to me. 
> It checks for `+1`, but the comment implies //two//.
> 
> (There should be only one allocation, btw).
> 
Oops, meant to get rid of this. 


================
Comment at: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.obs/op_bool.pass.cpp:39
+
+        if (std::shared_ptr<B> sp = std::dynamic_pointer_cast<B>(basePtr))
+        { check = true; }
----------------
mclow.lists wrote:
> This seems convoluted. Why not:
> ```
> std::shared_ptr<B> sp = std::dynamic_pointer_cast<B>(basePtr);
> assert(sp);
> ```
Yes, that would be simpler. I will update. 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62103/new/

https://reviews.llvm.org/D62103