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

[Alexander Richardson]

2014-04-16 20:17 GMT+02:00 Alexander Richardson
<arichardson.kde at googlemail.com>:
> 2014-04-15 19:23 GMT+02:00 Olivier Goffart <olivier at woboq.com>:
>> On Tuesday 15 April 2014 09:47:13 Thiago Macieira wrote:
>>> Em ter 15 abr 2014, às 02:36:58, Alexander Richardson escreveu:
>>> > I didn't see see Thiago's message since I am not subscribed to kfm-devel.
>>> > From what I saw in the Qt documentation I always assumed they were
>>> > equivalent (except for not allowing default arguments in the slot), which
>>> > other differences exist?
>>>
>>> The major difference is the problem of what gets called when the target
>>> object is being destroyed. Olivier has more details.
>>>
>>> Olivier?
>>
>> In general, if it compiles, there is no difference.
>>
>> But there is some things to take care of:
>> Using the string syntax, signals and slot behave as virtual, which means that
>> the connection will use signals or slots in the most derived class with the
>> same name if one exist even while connecting to the base class.  While the Qt5
>> syntax really connects to the slot of the class specified.  In order to get
>> the same behaviour, you need to mark the slot as virtual explicitly.
>>
>> And as Thiago mentioned there is also the problem that if a signal is emitted
>> from a destructor and is connected to a slot of the object being destroyed,
>> the slot will be called on the partially destroyed object.  This is more like
>> a bug but i have no clear idea how to fix it.
>>
>> --
>> Olivier
>>
>> Woboq - Qt services and support - http://woboq.com - http://code.woboq.org
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: main.cpp
Type: text/x-c++src
Size: 960 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140416/0bcc6a2c/attachment.cpp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: main.h
Type: text/x-chdr
Size: 2329 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140416/0bcc6a2c/attachment.h>