0

I'm having issues designing from a C# perspective.

I've got a class that returns a raw pointer to a class. Using a contrived example:

class Subject
{
  private:
   Topic _topic;
  public:
    void  AddTopic(Topic* topic)
    {
      delete _topic;
      _topic = topic;
    }
    void Update()
     {}

  ~Topic(){
       delete _topic;
   }
}

class Student
{
  vector<Topic*> _topicsCache;
  
  private:
    void DoSomething()
    {
      for(const auto & x: _topics)
      {
          x->Update(); //code segfaults
      }

    }
}

In the original code at some point somewhere, the Subject class gets deleted.

I'm thinking of refactoring the code without being too invasive to use a shared_ptr instead. Unfortunately, shared_ptr doesn't provide a callback to let me know when I can delete from the topics cache.

I was also thinking of a TopicsCache class to track all the Topics manually. The Topic would take a reference to the cache and remove itself when its destructor it's called. So:

class Topic
{
public:
  Topic(TopicsCache &topicCache);
  ~Topic()
  {
     topicCache.Remove(this);
  }
}
class TopicsCache
{
    void Remove(Topic* topic)
    {
        subjects->Remove(topic);
    }
}

Is this frowned upon in C++ land?

Lews Therin
  • 10,907
  • 4
  • 48
  • 72

2 Answers2

2

Hard to say without seeing how you use your topics cache. Have you considered storing weak pointers in your cache? As long as one other shared pointer exists you can upgrade a weak_ptr to a shared_ptr, otherwise you will get a nullptr.

Using the cache would be something like:

class TopicCache {
    std::map<std::string, std::weak_ptr<Topic>> _cache;
    public:
          std::shared_ptr<Topic> get(const std::string& name) {
              std::weak_ptr<Topic> weak = _cache[name];
              std::shared_ptr<Topic> ret = weak.lock(); // Try to convert to shared_ptr
              if (ret) {
                  return ret;
              } else {
                  ret = std::make_shared<Topic>(name);
                  _cache[name] = ret; // will store a weak reference to ret
                  return ret;
              }
          }
};
Botje
  • 26,269
  • 3
  • 31
  • 41
  • Hi thanks. This is how I envisioned using the cache. But the issue still remains, when Subject is disposed, how do I know when to clear `Topic` from the `TopicsCache` and `Student` vector? The issue is I don't know how to know clear from the `Student._topics` vector. Are you suggesting passing `TopicsCache` to `Student` then? – Lews Therin Aug 10 '20 at 10:07
1

The only drawback I see with your design is that Topic now needs to know where it's cached. From a design perspective this looks flawed, so my suggestion is to use a custom deleter for this. This way you move the relation between Topic and TopicsCache to wherever a Topic is created. Let's say your cache is also responsible for creating Topics. You could write something like this:

std::shared_ptr<Topic> createTopic()    
{
    // declare a custom deleter which is invoked when the last shared pointer is destroyed
    auto deleter = [this](Topic* ptr)
    {
        myOnDeleteEvent(*ptr);
        delete ptr;
    };

    return std::shared_ptr<Topic>(new Topic{}, deleter);
}

Here is a full example.

Timo
  • 9,269
  • 2
  • 28
  • 58