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.
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?
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?
...
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?
- line 53: You're assigning the key as the value, correct?
Yes.
...
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.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
