Review Request 117478: Convert dolphin (frameworks) to Qt5 signal/slot syntax

[Olivier Goffart]

On Wednesday 16 April 2014 20:47:10 Alexander Richardson wrote:
> 2014-04-16 20:17 GMT+02:00 Alexander Richardson

> > Ah yes, I completely forgot about slots being pseudo-virtual functions.
> > 
> > Would comparing the value of metaObject() be a possible solution to
> > calling slots from partially destructed objects?
> > Not sure if my approach in this test here is too naive, but it seems
> > to work. Maybe it breaks with other compilers than GCC or multiple or
> > virtual inheritance, I don't know that. To me it seems that the slot
> > is not invoked with the old syntax because the slot is not found
> > inside metaObject(), but maybe I am wrong, I didn't dig too deeply
> > into the metaobject source code.
> > 
> > In what I gathered from
> > http://woboq.com/blog/how-qt-signals-slots-work-part2-qt5.html a
> > QMetaObject* could be stored in QSlotObjectBase and compared to
> > receiver->metaObject() in order to determine whether the object is
> > being destroyed. Maybe there is a better place to store it as well
> > since adding another pointer to each signal slot connection sounds
> > expensive. Maybe inside QObjectPrivate, not sure if that is possible.
> > 
> > I think this should work since the destructor changes the vtable
> > pointer from what I found in my testing and googling, but I can't
> > quote the standard on it.
> > 
> > Assuming this fix for calling slots from the destructor after the
> > object has been destroyed the automatic conversion should be safe,
> > right?
> > Of course there needs to be a static analysis tool that makes sure
> > that no slots with the same name and parameter as a base class slot
> > are declared if they are not virtual, but that shouldn't be too
> > complicated using clang.
> > 
> > Regards,
> > Alex
> 
> I updated the test case, this should also allow valid calls from the
> destructor, but would definitly require storing the metaObject used at
> the time of connection to be stored inside the connection metadata
> (QSlotObjectBase).
> This would mean storing
> &QtPrivate::FunctionPointer<Func1>::Object::staticMetaObject; in
> checking in a loop over the superclasses if this MetaObject is equal
> to metaObject();
> Still not sure if this is correct, but it seems to be better than the
> previous solution
> 
> Regards,
> Alex

Yes.
As a matter of fact, we do something very similar in the classic syntax case: 
   if (... && c->method_offset <= receiver->metaObject()->methodOffset())
http://code.woboq.org/qt5/qtbase/src/corelib/kernel/qobject.cpp.html#3674

However we really want the best performance when emitting a signal, and i am 
afraid that one more virtual call (to get the meta is not neglectible).
But we probably will need to do something similar to what you did.

My idea was to store the metaobject in the QObjectPrivate::Connection.  But 
storing it in QSlotObject is also a good idea.  The only problem is that 
adding more code in QSlotObject means that the code will be duplicated for 
every possible template instance. But the advantage is that it will not slow 
down at all the lambda case.


About the lambda case, this is another problem i was discussing with Dario 
some times ago:
 connect(sender, &Sender::sig, context, [=]{context->doSomething(foobar);});
In that case, we do not know what MetaObject context really need to have.  So 
I guess for this case there is nothing that can be done.  But that should not 
keep us away from fixing the other cases.

-- 
Olivier

Woboq - Qt services and support - http://woboq.com - http://code.woboq.org