search

pytries / hat-trie

HAT-Trie for Python
MIT License
86 stars 21 forks source link

Support object types as values. #8

Closed b4hand closed 10 years ago

b4hand commented 10 years ago

Fixes #7.

kmike commented 10 years ago

Getting all these INCREFs / DECREFs right is tricky; I don't have much experience with them and haven't checked your implementation in details yet. For example, according to Wiki, <object> o should increase the refcount, but you're also increasing it. I think it is better to inspect the generated C code (check the generated html file), and maybe even add some tests for garbage collection.

b4hand commented 10 years ago

You're right about the extra Py_XINCREF because of the <object> o cast. I had read the docs that stated the cast implies an INCREF, but must have missed that invocation.

b4hand commented 10 years ago

FWIW, I was able to test with Python 2.7 and Python 3.2 on my machine using virtualenv, and they both worked. I can't seem to get your tox setup to work locally, so I wasn't able to run it all combined.

b4hand commented 10 years ago

Technically the change for 6475ecf is a bug in the original code. I bet if I insert a large enough integer value into the Trie and then try to get it back, the value will be truncated.

I can push that change as a separate PR with a single test if you'd prefer it.

b4hand commented 10 years ago

I think I've addressed all your comments including the leak check test. (Writing that test actually caused me to discover a bug I introduced in the 8b96e7b commit.)

kmike commented 10 years ago

The PR looks very good, thanks!