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