[Numpy-discussion] Ticket review: #848, leak in PyArray_DescrFromType

Travis E. Oliphant oliphant@enthought....
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 
> wrong.
>
>   
 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 
the code. 

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 
helpful. 

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.


-Travis




More information about the Numpy-discussion mailing list