Review needed: proposed fix for 2.6 __hash__ incompatibility (issue 2235)
I've posted a possible fix for the __hash__ backwards incompatibilities described in issue 2235 [1]. The patch uses a model similar to that used in Py3k (using None is indicate "don't inherit __hash__"), but extends it to allowing Py_None to be explicitly stored in the tp_hash slot. The major downside is that we suffer the cost of an extra pointer comparison on every call to PyObject_Hash, but I wasn't able to come up with another solution that preserved backwards compatibility while still allowing collections.Hashable to function correctly. The patch involves a few changes to fairly deep components in typeobject.c though, so I'd like at least some kind of sanity check before I commit it. Cheers, Nick. [1] http://bugs.python.org/issue2235 -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
On Wed, Jul 2, 2008 at 5:31 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
I've posted a possible fix for the __hash__ backwards incompatibilities described in issue 2235 [1].
The patch uses a model similar to that used in Py3k (using None is indicate "don't inherit __hash__"), but extends it to allowing Py_None to be explicitly stored in the tp_hash slot. The major downside is that we suffer the cost of an extra pointer comparison on every call to PyObject_Hash, but I wasn't able to come up with another solution that preserved backwards compatibility while still allowing collections.Hashable to function correctly.
From your description it seems storing Py_None in the slot acts as a magic value meaning "this is defined but not usable". However it used to be pretty common for various code around to call various slots directly (after a NULL) check. That would have disastrous results if the slot value was Py_None. Would it be terribly inconvenient if the magic value was in fact another function, with a public name), whose sole purpose was to raise an exception?
The patch involves a few changes to fairly deep components in typeobject.c though, so I'd like at least some kind of sanity check before I commit it.
Cheers, Nick.
I can't promise I'll have time to look at this before my EuroPython keynote, but it's important for me to get it right, so if nobody else jumps in, remind me Tuesday. -- --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
On Wed, Jul 2, 2008 at 5:31 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
I've posted a possible fix for the __hash__ backwards incompatibilities described in issue 2235 [1].
The patch uses a model similar to that used in Py3k (using None is indicate "don't inherit __hash__"), but extends it to allowing Py_None to be explicitly stored in the tp_hash slot. The major downside is that we suffer the cost of an extra pointer comparison on every call to PyObject_Hash, but I wasn't able to come up with another solution that preserved backwards compatibility while still allowing collections.Hashable to function correctly.
From your description it seems storing Py_None in the slot acts as a magic value meaning "this is defined but not usable". However it used to be pretty common for various code around to call various slots directly (after a NULL) check. That would have disastrous results if the slot value was Py_None. Would it be terribly inconvenient if the magic value was in fact another function, with a public name), whose sole purpose was to raise an exception?
Not only not inconvenient, but a significant improvement - as well as addressing your concern that I missed some code that calls tp_hash directly (a concern that I share, particularly since it could be an extension module we don't control that ends up doing it), it also gets rid of that extra pointer comparison in PyObject_Hash that was bothering me.
The patch involves a few changes to fairly deep components in typeobject.c though, so I'd like at least some kind of sanity check before I commit it.
Cheers, Nick.
I can't promise I'll have time to look at this before my EuroPython keynote, but it's important for me to get it right, so if nobody else jumps in, remind me Tuesday.
I'd now advise waiting until I have a chance to implement your idea anyway :) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
participants (2)
-
Guido van Rossum -
Nick Coghlan