> * Probably makes no difference, but it seems oddly asymmetric to fiddle with > the interned string's refcount in string_dealloc, call PyObject_DelItem, > then not restore the refcount to zero. That's unnecessary because the next line executed simply frees the object. free() doesn't check the refcount. It's a bit optimistic in that it doesn't check the DelItem for an error; but that's a separate issue, and I don't know what it should do when it gets an error at that point. Probably call Py_FatalError(); if it wanted to recover, it would have to call PyErr_Fetch() / PyErr_Restore() around the DelItem() call, because we're in a dealloc handler here and that shouldn't change the exception state. > * Should be Py_DECREF(keys) (not Py_XDECREF(keys)) in > _Py_ReleaseInternedStrings. If you've gotten that far keys can't be > NULL. If you're worried about keys being NULL, you should check it before > the for loop (PyMapping_Size() will barf on a NULL arg). You're right. Also, I think it should use PyDict_Keys() and PyDict_Size() -- it knows that interned is a dict so all the hoopla that PyMapping_Keys() adds is unnecessary. Maybe the best thing to do is to remove _Py_ReleaseInternedStrings() and let Barry worry about how to implement it the next time he wants to use Insure++. > Also, regarding the name of PyString_InternInPlace, I see now that's the > original name. I suggest that name be deprecated in favor of > PyString_InternImmortal with a macro defined in stringobject.h for > compatibility. Yeah, if we keep the immortality feature at all. BTW, it looks like Oren was careless with error checking in a few places. The whole patch needs to checked carefully. --Guido van Rossum (home page: http://www.python.org/~guido/)
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4