Can make_shared occasionally create duplicate pointers?

Question

I'm failing some of my unit tests about 25% of the time per test. The problem is that when I create an "Entity" object, I use the std::make_shared function to create a pointer to another object. It seems that occasionally, pointers in separate Entity objects point to the same transform object. I'm not sure how this is possible. Here's the relevant code:

Entity constructor:

Entity::Entity(std::shared_ptr<Shape> i_shape) :
    shape {i_shape}
{
    transform = std::make_shared<Transform> (Vector2f(0.0f, 0.0f), Vector2f(1.0f, 1.0f), 0.0f);
}

Visual Studio 2017 Unit test code revealing the issue:

        TEST_METHOD(CircleCollisionTest2)
        {
            Entity* testCircleEnt1;
            Entity* testCircleEnt2;
            std::shared_ptr<Circle> testCircle1 = std::make_shared<Circle>(60.0f);
            std::shared_ptr<Circle> testCircle2 = std::make_shared<Circle>(60.0f);
            testCircleEnt1 = &Entity(testCircle1);
            testCircleEnt2 = &Entity(testCircle2);
            testCircleEnt1->GetTransform()->SetPostion(Vector2f(0, 0));
            testCircleEnt2->GetTransform()->SetPostion(Vector2f(200, 0));

            Assert::IsFalse(Physics::TestCollision(testCircleEnt1, testCircleEnt2));
        }

When I instantiate my Entity objects, make_shared is creating pointers to the same Transform objects. I've confirmed that they hold the same address in debug mode, so it shouldn't involve my getters or setters or anything like that.

Also, whenever this happens, the failing test takes ~150ms to complete rather than ~5ms. How can be happening roughly 25% of the time rather than 100% or 0%? I thought it could be related to the Visual studio testing framework but I replicated these results in a custom made test.

Any help would be very welcome. Thanks!

score 2 · Accepted Answer · answered Apr 07 '20 at 18:54

Your code code has undefined behavior. When you do

testCircleEnt1 = &Entity(testCircle1);
testCircleEnt2 = &Entity(testCircle2);

You create two temporary objects and store the address of those. First, that should not compile. If it is, you need to turn up your wanring/error options. Second, temporary objects are destroyed at the end of the full expression they are in so that leaves you with two dangling pointers. What your code should be is

TEST_METHOD(CircleCollisionTest2)
{
    std::shared_ptr<Circle> testCircle1 = std::make_shared<Circle>(60.0f);
    std::shared_ptr<Circle> testCircle2 = std::make_shared<Circle>(60.0f);
    testCircle1->GetTransform()->SetPostion(Vector2f(0, 0));
    testCircle2->GetTransform()->SetPostion(Vector2f(200, 0));

    Assert::IsFalse(Physics::TestCollision(testCircle1.get(), testCircle2.get()));
}

Since you are using visual studio, you should turn on the /permissive- to enfore stronger compliance to the C++ standard.

I really appreciate the help. I did not even know I was creating temp objects. Thanks! — john_shreds, Apr 07 '20 at 19:09

score 1 · Answer 2 · answered Apr 07 '20 at 18:53

1

It's okay, it's just you create two dangling pointers by taking addresses of temporaries and then even dereference them, invoking an undefined behaviour which you observe.

answered Apr 07 '20 at 18:53

bipll

11,747
1
18
32

Oh awesome thank you so much! Where do I create these dangling pointers? Is it when I create the transform? I thought the point of make_shared was that it wouldn't be "dangling". The "transform" in my constructor is a class attribute (**std::shared_ptr transform;**) – john_shreds Apr 07 '20 at 18:56

score 0 · Answer 3 · answered Apr 07 '20 at 18:56

I'm surprised that this code compiles, as it isn't legal C++:

testCircleEnt1 = &Entity(testCircle1);
testCircleEnt2 = &Entity(testCircle2);

Here, the expression Entity(testCircle1) produces a temporary Entity object, and you can't take the address of temporary objects in C++. Were you to do so, you'd run into trouble because temporary objects in C++ cease to exist after the statement that creates them finishes running, leaving the pointers pointing to objects that no longer exist. That leads to undefined behavior, hence your periodic crashes.

If you'd like to create new objects to have testCircleEnt1 and testCircleEnt2 point at, you could use new:

testCircleEnt1 = new Entity(testCircle1);
testCircleEnt2 = new Entity(testCircle2);

However, at this point you'd probably be better off either

just declaring testCircleEnt1 and testCircleEnt2 as actual Entity objects rather than pointers to them, or
declaring testCircleEnt1 and testCircleEnt1 as std::shared_ptr<Entity>s rather than raw pointers.

I literally had no idea I was creating temporary objects. I didn't know that using the constructor with the "&" made it a temporary. Thank you very much. — john_shreds, Apr 07 '20 at 19:07
I honestly don't know which answer to mark as correct, they are both very helpful and useful. I just marked the one with the most votes. Thank you very much — john_shreds, Apr 07 '20 at 19:08

Can make_shared occasionally create duplicate pointers?

3 Answers3