[Kde-bindings] Problem condition in Qyoto MethodCall

[Arno Rehn]

Am Montag, 12. Februar 2007 schrieb Phil Thompson:
> On Monday 12 February 2007 5:13 pm, Arno Rehn wrote:
> > Am Montag, 12. Februar 2007 schrieb Richard Dale:
> > > On Monday 12 February 2007, Arno Rehn wrote:
> > > > > [ ... cut ... ]
> > > >
> > > > Well, I've hacked a lot around in qyoto.cpp and changed the way we
> > > > check if it's a contained instance or not: I've added a field to
> > > > smokeqyoto_object called 'contained' and it is set up when the
> > > > constructor was just called. This was necessary because sometimes the
> > > > instance was already destroyed and we still wanted to get the parent
> > > > of it. The new field is updated when the parent of the widget is
> > > > changed because it is added to a layout, too. It works better now and
> > > > up to t10 every tutorial works well. But with t11 it keeps crashing
> > > > and I can't figure out why it does so. Since I've created quite a
> > > > mess, I don't want to check in what I have. I've attached a patch
> > > > with all the changes, if you could take a look at it and tell me what
> > > > you think...
> > >
> > > I'm not sure if this is the best way to do it, as 'contained' seems to
> > > duplicate the function of 'allocated'. If an instance is 'owned' by
> > > another instance we can just set 'allocated' to false, and the Qyoto
> > > runtime won't delete it when the corresponding C# instance is garbage
> > > collected.
> >
> > Yes, didn't think about that, but it's right. Setting allocated to false
> > would be better.
> > Do I see that right: If allocated has to be true for a destructor to be
> > called, we only want to call destructors for instances which have been
> > created by a C# constructor? Then we'd just have to check if it's a
> > destructor and if getPointerObject returns something other than 0.
>
> I don't think so. Whether an instance is currently 'contained' has little
> to do with whether it was created by a C# ctor. An instance can be
> contained or not contained at different times depending on how it is used.
> You only call the dtor if it is not contained. You must correctly maintain
> the state of whether an instance is currently contained. If you don't you
> will either get crashes (when there are multiple calls to the dtor) or
> memory leaks (when there are no calls to the dtor). This is why automated
> bindings generators only get you so far. If you want rock-solid bindings
> you have to look at every API call yourself.
Indeed, I've just come across this. It's better to keep the "contained" field 
in smokeqyoto_object. First off it's easier to understand and second you may 
want to do something else with the value of "allocated", and then it has to 
have the right 'meaning'.

Another problem I encountered was that the 'deleted' method in 
QyotoSmokeBinding seems to be only called for classes derived from 'QObject', 
e.g. when a QRect is destroyed the method is not invoked. That makes it more 
difficult to handle the weakRef map correctly.

> > > What problem does it solve that the current scheme with the
> > > IsContainedInstance() function doesn't do?
> >
> > The problem is the following: We have a QLabel which is a child to a
> > QWidget, for example. Now the QWidget is destroyed and with it the QLabel
> > is gone. Nevertheless the Qyoto runtime wants to call the destructor for
> > the QLabel as it still 'thinks' the QLabel exists. To check whether
> > QLabel is a child to something and whether to call the destructor we call
> > IsContainedInstance() which checks for any parents. Thus it will cause an
> > error when trying to call QLabel->parent() as the QLabel is already
> > destroyed. Therefore it's better to set this property up when the
> > instance is created and when the parent is changed.
> >
> > > I don't think there's any hurry to solve the problem straight away, and
> > > it's best to keep experimenting like you're doing, but not actually
> > > commiting anything.
> >
> > Yes, that's what I thought, too.
> >
> > > If we use the 'Transfer' data in the PyQt sip files we would need to
> > > change ownership when a method, or argument within a method was marked
> > > with 'Transfer'. So that data could either go into the smoke library
> > > runtime, as it would be useful for every language. Another way would be
> > > to add it as C# Attributes, in the 'SmokeMethod' one perhaps.
> >
> > What exactly is this 'Transfer' data used in PyQt? Haven't heard of it
> > before...
>
> The different Transfer annotations are the means by which PyQt knows that
> an instance's 'contained' state has changed. For example...
>
>     QLabel(QWidget *parent /TransferThis/ = 0, Qt::WindowFlags f = 0);
>
> ...means that the QLabel instance is contained if parent is non-zero,
> otherwise it is not contained. "Transfer" is short for "transfer
> responsibility for calling the dtor".
I see, but I don't think it can be implemented with attributes in C#. If we 
want to do it in C# we would have to catch the calls in 
SmokeInvocation.Invoke() and then decide whether to set 'contained' to true 
or not. Though I think it's better to keep this whole thing in C++, otherwise 
it would require more and more calls from C# to C++ and thus slow down the 
whole thing.


-- 
Arno Rehn
arno at arnorehn.de