On Fri, Apr 02, 2010 at 05:14:19PM -0500, Shawn Walker wrote:
> On 04/ 2/10 04:46 PM, [email protected] wrote:
> >On Sun, Mar 28, 2010 at 08:31:16PM -0500, Shawn Walker wrote:
> >>webrev:
> >>http://cr.opensolaris.org/~swalker/pkg-slots/
> >
> >_actions.c:
> >
> > - The reference counting here needs to be changed. The typical
> > calling convention for Python C modules is that the callee borrows
> > the references from the caller, unless explicitly documented
> > otherwise.
>
> Yes, but I was trying to avoid adding an extra function or in-lining
> the logic for getting the cached attribute from the dictionary.
> That's why the inc/dec logic was added to add_to_attrs -- it seemed
> logical to do it one place. The python documents seem to be
> inconsistent about recommendations for when it's safe to borrow
> references, and when it's not.
I don't think what I'm suggesting will cause you to need to add an extra
function, or inline anything else. I'm just asking that you move the
INCREF and DECREF calls, since there's no documentation about the
calling convention. IMO, the current way that the references are
handled is pretty confusing and my hope is that what I'm suggesting
simplifies the understanding of this code for others who read it.
> > The add_to_attrs() function is actually freeing the caller's
> > references in the case when it returns an error. I think this is
> > much more confusing than necessary, and creates problems in the
> > code that you're adding. I would do the following:
>
> So to be clear, previously, the callers wouldn't have decref'd at
> all in an error case. As such, it seems you're primarily concerned
> with moving the decref logic to callers to make it clearer who's
> responsible or to avoid the caller and this function both doing it?
Yes, that's exactly it. The _fromstr(), the caller of add_to_attrs(),
decref's everything else that it creates except for the attributes it
passes to add_to_attrs. My suggestion is that it manage those too,
which simplifies what happens in add_to_attrs(), especially as we add
code to that routine.
> > a. Have the callers of add_to_attrs() Py_DECREF attrs, key, and attr
> > when they return in error.
>
> To be clear, when add_to_attrs() returns in error, you want me to
> have the callers do that?
The current code looks like this:
if (add_to_attrs(attrs, key, attr) == -1)
return (NULL);
I'm suggesting this:
if (add_to_attrs(attrs, key, attr) == -1) {
Py_[X]DECREF(attrs);
Py_[X]DECREF(key);
Py_[X]DECREF(attr);
return (NULL);
}
Then you also have to make sure that key and attr are DECREF'd before
the return in the default case on line 335.
> ...
> > c. Change add_to_attrs() to only Py_DECREF references that it has
> > INCREF'd or taken as the result of a function call.
> >
> > There's a further caveat when assigning memo_entry to key on line
> > 48: you now have key with a reference that's been added by this
> > function. I believe that to solve the problem with uneven reference
> > counts, you need to Py_INCREF(key) before line 43 so that subsequent
> > DECREF's don't get the reference count confused.
>
> Yes, that was intentional. I don't quite follow the uneven
> reference counts bit though. Although looking at this, I can see
> other problems, but nothing that points to uneven reference counts.
> Can you expound a bit?
Sorry, what I meant is that if you make the changes so that _fromstr()
is supposed to DECREF key, then you need to INCREF key on line 43. This
is to account for the cases where key is not reassigned to memo_entry.
> >actions/
> >
> > - General question: Do the actions subclasses inherit the slots values
> > from their parents? If not, shouldn't all of the cases where
> > __slots__ = [], contain the set of attributes that's common to all
> > actions?
>
> No; __slots__ are weird because sub-classes are only supposed to
> indicate any *additional* attributes that the parent class has not
> named already.
Ok, that's what I assumed was going on, but I wanted to double check.
Thanks for clarifying.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
