[Numpy-discussion] Ticket review: #848, leak in PyArray_DescrFromType
Travis E. Oliphant
Sat Jul 19 13:27:40 CDT 2008
Michael Abbott wrote:
> I'm not actually convinced by the comment that's there now, which says
> /* typecode will be NULL */
> but in truth it doesn't matter -- because of the correcly placed DECREF
> after the PyArray_Scalar calls the routine no longer owns typecode.
I'm pretty sure that it's fine.
> If I can refer to my last message, I made the point that there wasn't a
> good invariant at the finish label -- we didn't know how many references
> to typecode we were responsible for at that point -- and I offered the
> solution to keep typecode. Instead you have chosen to recreate typecode,
> which I hadn't realised was just as good.
I agree that this routine needs aesthetic improvement. I had hoped
that someone would have improved the array scalars routines by now.
I think a core issue is that to save a couple of lines of code, an
inappropriate goto finish in the macro was used. This complicated the
code more than the savings of a couple lines of code would justify.
Really this code "grew" from a simple thing into a complicated thing as
more "features" were added. This is a common issue that happens all
over the place.
> This code is still horrible, but I don't think I want to try to understand
> it anymore. It'd be really nice (it'd make me feel a lot better) if you'd
> agree that my original patch was in fact correct. I'm not disputing the
> correcness of the current solution (except I think that typecode can end
> up being created twice, but who really cares?) but I've put a lot of
> effort into arguing my case, and the fact is my original patch was not
From what I saw, I'm still not quite sure. Your description of
reference counting was correct and it is clear you've studied the issue
which is great, because there aren't that many people who understand
reference counting on the C-level in Python anymore and it is still a
useful skill. I'm hopeful that your description of reference
counting will be something others can find and learn from.
The reason I say I'm not sure is that I don't remember seeing a DECREF
after the PyArray_Scalar in the obj = NULL part of the code in my
looking at your patches. But, I could have just missed it.
Regardless, that core piece was lost in my trying to figure out the
other changes you were making to the code.
From a "generic" reference counting point of view you did correctly
emphasize the problem of having a reference count creation occur in an
if-statement but a DECREF occur all the time in the finish: section of
It was really that statement: "the fantasy that PyArray_Scalar steals a
reference" that tipped me off to what I consider one of the real
problems to be. The fact that it was masked at the end of a long
discussion about other reference counting and a "stealing" discussion
that were not the core problem was distracting and ultimately not very
I'm very impressed with your ability to follow these reference count
issues. Especially given that you only started learning about the
Python C-API a few months ago (if I remember correctly). I'm also
very glad you are checking these corner cases which have not received
the testing that they deserve. I hope we have not discouraged you too
much from continuing to help. Your input is highly valued.
More information about the Numpy-discussion