Solved emitting signal causes destructor call
-
Hi all -
So, I have a program with a worker routine as follows:
void Worker::run() { int len; running.ref(); // set to value of 1 while (running) { len = sm.recv(buffIn, sizeof(buffIn)); if (len >= 0) { buffIn[len] = '\0'; Message msg(MSG_UNKNOWN, buffIn); emit(newMessage(msg)); } Sleep(10); } emit reachedEndOfThread(); }
Message code is:
Message::Message(MsgType type, char *buffer) : m_type(type) { . . . hash = new(XmlHash); } Message::~Message() { delete hash; }
The emit in Worker::run() causes the ~Message() to be called twice (creating a seg fault on the 2nd call). Any ideas?
Thanks...
-
The
Message
instance is being copied at the emission point because (I assume) your connection is of queued type.The way to fix it is to provide a proper copy constructor for the
Message
class, as the automatically generated one will simply copy thehash
pointer and thus make it share the same data among two instances, which will crash on the second one's destructor. -
Oh man...what a rookie mistake.
As long as I have your attention, how would you recommend implementing the section of the copy constructor that deals with the unordered_map member? I've gotten as far as:
XmlHash::iterator it; hash = new(XmlHash); for (it = from.hash->begin(); it != from.hash->end(); ++it) { }
but I'm unsure how to use the iterator to retrieve the hash key. Once I have the key, I can get the value easily enough (I think).
Thanks...
-
Hi,
Since it's an
std::unordered_map
, the iterator returns astd::pair
containing the key and the value. -
Hi SGaist -
Thanks for the refresher on that. I still struggle with the operators for this class.
I think the easiest way to copy the entire table is:
Message::Message(const Message &from) { hash = new(XmlHash); hash->insert(from.hash->begin(), from.hash->end()); }
-
Why not use the copy constructor of
std::unordered_map
? -
If hash were simply an unordered_map, I could just do:
hash = from.hash;
right? But I don't know how to code that when hash is a pointer.
-
hash = *otherHashPointer;
But why is it a pointer in the first place ?
-
As @SGaist pointed out you could make it a non-pointer. This would spare you implementing a copy constructor and a destructor, as the automatically generated ones would do the right thing.
If, however, you want that pointer anyway, then you can just use map's copy constructor:
hash = new XmlHash(*from.hash);
Btw. This is unrelated but I'm just curious - what's with the weird syntax in
new(XmlHash)
oremit(newMessage(msg))
? What do you need those extra () for? -
You know, that's a damned good question. It no longer is a pointer. (Of course, now I no longer need the copy constructor, but I'm going to keep it since I struggled so much with it.)
Chris: no good reason for the extra parens...they're probably relics of various attempts at getting this working. They too are gone now.
Thanks, guys.