Pointer parameter in signals: Memory leak?

Hello,

Code:

---

class Sender : public QObject
{
    Q_OBJECT
 
public:
   Sender(int val) : mVal(val) { }
 
signal:
    void intChanged(ChangeObject*);
 
public slots:
    void changeInt(int i) {
        mVal = i;
        emit intChanged(new ChangeObject(mVal));
    }
 
private:
    int mVal;
};
 
class Receiver : public QObject
{
    Q_OBJECT
 
public:
    Receiver();
 
public slots:
    void onIntChanged(ChangeObject* c) {
        mReceivedVal = c->changed();
    }
 
private:
    mReceivedVal;
};
class ChangeObject
{
public:
    ChangeObject(int changed) : mChanged(changed) { }
 
    int changed() const { return mChanged; }
private:
    int mChanged;
};

---

Where to delete new created ChangeObject? in class Receiver? What about not handled signals with pointer parameter types? (like the Qt own signals that are not accepted ie e->accept() is not called in receiver object)

Thanks for any clarification

It's impossible to answer your question, because we can't figure out if the caller or callee is responsible or not.

Unless you use queued connections, a signal-slot connection is just a function call. So it should be safe to delete the pointer at the end of the slot. If this is the case, I'd use auto_ptr. (Not that I would ever emit pointers unless I was absolutely forced to.) But if you are using queued connections, which is the case if it's across threads, then you have no idea if it's safe or not.

Instead, you should recode your application to emit something that is not a pointer. In the code you offer, the pointer is only used to get a bool. In that case, emit the bool directly.

If you are worried about performance, in case your object isn't just a bool but might be huge, you can create your own implicitly shared object type and have copy-on-write. If you have coded this const-correctly, that should fix your problems.

You can also emit a QSharedPointer<whatever*>, but you might have to register this type to QVariant.

Of all these solutions, there is only one that really makes sense: Don't emit pointers.

Hello Bo,

thanks for your answer.

Of course in reality the signal parameter is meant to be an event object. I have a class hierarchy of special event objects:

Code:

---

class Event
{
public:
    enum EventType
    {
        ET_Message,
        ET_Item,
        ET_Action,
        [...]
    };
 
    Event(EventType t);
 
private:
    EventType mEventType;
};
 
class MessageEvent : public Event
{
public:
    MessageEvent(Event::EventType t = Event::Message);
 
    QString message() const {
        return mMessage;
    }
 
private:
    QString mMessage;
};
 
class ItemEvent : public Event
{
public:
    ItemEvent(Item i, Event::EventType t = Event::Item);
 
    Item item() const {
        return mItem;
    }
 
private:
    Item mItem;
};

---

In my general state class I have a general signal to emit all types of events, any type of event can be assigned to any state:

Code:

---

class State : public QObject
{
    Q_OBJECT
public:
    State(QObject* parent = 0);
 
    void addEvent(KeyEvent k, Event* e);
 
signals:
    void event(Event*);
 
public slots:
   void handleEvent(KeyEvent k);
 
private:
    QHash<KeyEvent, Event*> mEventTable;
};
 
 
void State::handleEvent(KeyEvent k)
{
    // emit event for this key event
    if (mEventTable.contains(k))
        emit event(mEventTable.value(k));
}

---

Events are registered with states at creation:

Code:

---

class MyOtherClass
{
 
    void initStates()
    {
        State* s1 = new State();
        s1->addEvent(KE_CursorDown, new ItemEvent(item1, Event::ET_Item));
        [...]
 
        register all state signals:
        connect(s1, SIGNAL(event(Event*)), this, SLOT(onEvent(Event*)));
        [...]
    }
};

---

So there are "polymorphic" events, so I have to use pointers I think. I want a general event signal for all sorts of events.

Thanks for any suggestions

I suggest to avoid calling your method "event" as there already exists a method with this name in QObject. As for the problem itself, I would probably try to avoid broadcasting your events with signals or at least avoiding pointers. There is a good chance at some point you forget what ownership rules you have set and delete the event in a wrong place. If you want to store the event objects somewhere then obviously you can't delete them in the context of the emitting method. Since you don't know how many objects may store the event you can't delete it in the receiver's context as well. So the only choice is to delete the object in the context of the object that created the event (or a global event manager) but you have to make sure no other context references your events. You can do it like so:

Code:

---

void EventManager::cleanupAllEvents() {
  emit eventsAboutToBeDeleted();
  qDeleteAll(m_events);
  emit eventsDeleted();
}

---

Then you can connect to the first signal with every object handling events and clear their own event caches.

You can also do it on an event-by-event basis:

Code:

---

void EventManager::cleanupEvent(Event *e){
  emit eventToBeDeleted(e);
  delete m_events.find(e, 0);
  m_events.removeOne(e);
  emit eventDeleted(e);
}

---

and handle it in receivers in a similar fashion as in the previous case.

But I'd suggest simply to avoid storing the events anywhere. It'd be best if you just used Qt's event system for your events, just subclass QEvent and use QCoreApplication::postEvent() to post your events.