[MERGE] (refreshed) pyrex iter-changes

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> Robert Collins wrote:
>> Just updating this patch to resolve NEWS issues.
> 
>> -Rob
> 
> 
> I haven't thoroughly reviewed your code yet, but I went ahead and
> updated it so that it will compile using mingw32 (and I would assume
> Visual Studio 2003, but I don't have that to test.)
> 
> I'm also including the incremental change as a diff.
> 
> This is built on top of my earlier python-compat.h changes.
> 
> John
> =:->
> 

Overall, I think I'd rather bring this in, and then tweak it from there,
rather than waiting to get it perfect before we merge it. What do you think?

I'd certainly like to take some time to evaluate the critical path of
"nothing changed" and the "something added but nothing else different".
I have the feeling our overhead is actually the Unicode upcast and
downcast to display overhead. But I'm guessing there.


@@ -41,10 +57,18 @@
     ctypedef int intptr_t


+
 cdef extern from "stdlib.h":
     unsigned long int strtoul(char *nptr, char **endptr, int base)


^- This superfluous whitespace was brought-to-life by me, and dangit,
I'll take it out.

...

+    ctypedef struct PyObject:
+        pass
     int PyList_Append(object lst, object item) except -1
     void *PyList_GetItem_object_void "PyList_GET_ITEM" (object lst, int
index)
+    void *PyList_GetItem_void_void "PyList_GET_ITEM" (void * lst, int
index)
+    object PyList_GET_ITEM(object lst, Py_ssize_t index)
     int PyList_CheckExact(object)
+    Py_ssize_t PyList_GET_SIZE (object p)

^- Now that you have revealed pyrex 0.9.7+ can handle this directly with
a "cdef list" or "cdef tuple" structure, I'd really push for making that
a requirement. It would lead to *much* prettier and easier to manage
pyrex code. I'll leave it alone for now, but consider a +1 from me on
upgrading the minimum.

...

v- As Alexander mentioned, we can use enums here:

+# This is the Windows equivalent of ENOTDIR
+# It is defined in pywin32.winerror, but we don't want a strong
dependency for
+# just an error code.
+# XXX: Perhaps we could get it from a windows header ?
+cdef int ERROR_PATH_NOT_FOUND
+ERROR_PATH_NOT_FOUND = 3
+cdef int ERROR_DIRECTORY
+ERROR_DIRECTORY = 267

I've provided better code already:

cdef enum:
	ERROR_PATH_NOT_FOUND = 3
	ERROR_DIRECTORY = 267


Though I will note it is a #define in "WinError.h":
#define ERROR_DIRECTORY                  267L



I find the structure of "_iter_next()" a bit odd. In that you have some
loops, and then "return result" out of them. You have a comment at the
top about it not being optimal. In the short-term it is probably fine,
but it certainly is a bit odd to follow into the code and figure out
where we are each time.

If you feel like, moving some of it out into helper functions would be
nice. For example, the if block with:
+        if self.current_dir_info is None and self.current_block is None:
+            # setup iteration of this root:


It would be nice to have the rest of that as a helper so you would ahve:

if self.current_dir_info is None and self.current block is None:
  self.setup_filesystem_iteration()

Similarly, stuff like "if self.current_root is None" seems like it could
be farmed out into a helper which would help make the function easier to
handle. I know we already have this big unwieldy thing, so it isn't like
you *have* to do it to get merged, but I think it would be a good way
forward.


...

+                        try:
+                            e_winerror = e.winerror
+                        except AttributeError:
+                            e_winerror = None
+                        win_errors = (ERROR_DIRECTORY,
ERROR_PATH_NOT_FOUND)
+                        if (e.errno in win_errors or e_winerror in
win_errors):
+                            self.current_dir_info = None
+                        else:
+                            # Will this really raise the right exception ?
+                            raise

^- This will sometimes raise the "AttributeError", I believe.

Can we use getattr? That might suffer the same problem. The other option
is to explicitly capture "sys.exc_info()" and raise that directly.


I should note, that 'bzr selftest -s bzrlib.tests.intertree' only has 1
failure, and it fails in bzr.dev. So I'm pretty sure the overall code is
correct (at least with python2.5).


Also, since this now compiles on win32, can we move the python form and
"de-tune" it?


Anyway, I think we could do a lot of farming out code into helper
functions and really aid clarity, but I'm comfortable that the code is
correct, and I'd rather have it than wait forever for cleanups.

BB:approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkjcObgACgkQJdeBCYSNAAOsRACeKCtrGMWEyGtIY6t7AJlFeKIu
qSgAn0CH46txi/+YDN9t+iCnaZMkazmc
=KRrR
-----END PGP SIGNATURE-----