1

In my program Groups will have shared pointers to Subjects; and Subjects will have weak pointers to their Groups. I want the Group to have a join() function that assigns the Subject's weak pointer to itself. Below is the minimal code for what I've tried. How do I fix the join() function?

#include <iostream>
#include <string>
#include <memory>

class Party;

class Subject
{
public:
    std::weak_ptr<Party> MyParty;
};

class Party
{
public:
    std::string Name;

    void join(std::shared_ptr<Subject> subject)
    {
        subject->MyParty = std::make_shared<Party>(*this); // <---- PROBLEM
    }
};

int main()
{
    auto& BlueParty = std::make_shared<Party>();
    BlueParty->Name = "Blue Party";

    auto& Jane = std::make_shared<Subject>();

    BlueParty->join(Jane);

    if (auto ptr = Jane->MyParty.lock())
    { 
        std::cout << "I am in " << ptr->Name << std::endl; 
    }

    else { std::cout << "I have no party." << std::endl; }

    return 0;
}

The program prints out "I have no party". If the assignment were successful, it should have printed out "I am in Blue Party".

Nathaniel G.M.
  • 433
  • 5
  • 15
  • 5
    It seems like you're looking for [`std::enable_shared_from_this`](https://en.cppreference.com/w/cpp/memory/enable_shared_from_this). – Some programmer dude Jun 14 '19 at 13:47
  • The problem is that Party is not self-aware that it is always owned by a shared_ptr. That's not necessarily a bad thing. One option would be to pass in the shared_ptr of the Party to join (could be made a static class function). – Eljay Jun 14 '19 at 13:55

1 Answers1

6

The line subject->MyParty = std::make_shared<Party>(*this); creates a new Party object that is a copy of *this and is managed by a temporary std::shared_ptr. subject->MyParty gets assigned from that temporary shared_ptr, but weak_ptrs don't keep the objects they point to alive. As soon as that statement completes, the temporary shared_ptr returned by make_shared is destroyed and takes the Party object it was managing with it. subject->MyParty now doesn't point to anything.

The solution is to use std::enable_shared_from_this:

class Party : public std::enable_shared_from_this<Party>
{
public:
    std::string Name;

    void join(std::shared_ptr<Subject> subject)
    {
        subject->MyParty = shared_from_this();
    }
};

Example

To use shared_from_this, the object must be owned by a std::shared_ptr. It's generally a good idea, in such a case, to mark the class's constructors private and use a factory function that returns a shared_ptr to a new instance so that objects of that type that aren't managed by a shared_ptr can't be accidentally created:

class Party : public std::enable_shared_from_this<Party>
{
public:
    std::string Name;

    static std::shared_ptr<Party> create()
    {
        return std::shared_ptr<Party>{new Party()};
    }

    void join(std::shared_ptr<Subject> subject)
    {
        subject->MyParty = shared_from_this();
    }
private:
    Party() = default;
    Party(const Party&) = delete;
};

Example

Sadly, this makes std::make_shared harder to use. For a bit more info on that issue see this question.

Miles Budnek
  • 28,216
  • 2
  • 35
  • 52
  • The danger of std::enable_shared_from_this is that now Party instances are self-aware they are owned by a shared_ptr. This may be a problem if a Party is created on the stack, but isn't a shared_ptr, and happens to be used in a such a way that something tries to get a shared_ptr or weak_ptr to it, which lands in UB land. To protect against that, use a factory function that returns a shared_ptr, and make the constructors inaccessible using a tag key (because you still want make_shared to work). – Eljay Jun 14 '19 at 14:03
  • @Eljay Added a section discussing that. – Miles Budnek Jun 14 '19 at 14:15