Regression due to PyDict_Next optimization

[robertwb]

These changesets introduce a code generation bug that causes repeated dictionary iteration to fail (using Python 2.6 on 32- and 64-bit Ubuntu 8.04):

changeset:   1344:7a102bc50f8f
user:        Stefan Behnel <scoder@users.berlios.de>
date:        Sun Nov 16 22:45:33 2008 +0100
summary:     integrate new iter-dict transform

changeset:   1343:db3eb81258f4
user:        Stefan Behnel <scoder@users.berlios.de>
date:        Sun Nov 16 22:45:12 2008 +0100
summary:     new transform that converts for-in-dict.iter*() into a while-loop over PyDict_Next(), which makes the loop 30-50% faster

The attached test case demonstrates the problem.

At 2008-11-19T21:38:59Z jasone added attachment foo.pyx

At 2008-11-20T18:30:58Z jasone added attachment PyDict_Next

Migrated from http://trac.cython.org/ticket/123

[robertwb]

jasone commented

The attached patch fixes iteration by using a local temporary variable, rather than using the iterator counter. I think the real problem is that some tree tranform is removing the node that initializes the iterator counter (perhaps it's viewed as a redundant global variable initialization), but finding that problem is beyond my understanding of Cython at this point.

[robertwb]

scoder changed owner from somebody to scoder
commented

[robertwb]

scoder changed owner from scoder to empty
commented

Yes, it looks like a problem with the initialisation. The same temp node gets reused here, but it's only initialised once.

I applied your work-around by now, but the real problem is elsewhere in the temp code.

[robertwb]

@dagss commented

I have now changed IteratorNode so that the counter is considered an implementation detail/"temp register", rather than a subnode in the tree. Thus it shouldn't be used by transforms, and the consequence is that this patch now does the entirely correct thing.

I recommend closing as "fixed", but waiting for review.

[robertwb]

@dagss commented

See http://trac.cython.org/ticket/124 for relevant patches.

[robertwb]

scoder changed resolution to fixed
status from new to closed
commented

Yes, that's the right thing to do.

[robertwb]

@robertwb changed resolution from fixed to empty
status from closed to reopened
commented

[robertwb]

@robertwb changed owner to jasone
status from reopened to new
commented

[robertwb]

@robertwb changed resolution to fixed
status from new to closed
commented