#22524: PEP 471 implementation: os.scandir() directory scanning function
|
Created:
4 years, 10 months ago by benhoyt Modified:
4 years, 5 months ago Reviewers:
berker.peksag, victor.stinner, storchaka, jimjjewett CC:
tebeka, AntoinePitrou, haypo, giampaolo.rodola, tim.golden, benhoyt, abacabadabacaba_gmail.com, akira, socketpair, devnull_psf.upfronthosting.co.za, Josh.R, alexei.romanov Visibility:
Public. |
Patch Set 1 #
Total comments: 64
Patch Set 2 #
Total comments: 3
Patch Set 3 #
Total comments: 49
Patch Set 4 #Patch Set 5 #
Total comments: 24
Patch Set 6 #Patch Set 7 #
Total comments: 65
Patch Set 8 #Patch Set 9 #MessagesTotal messages: 15
I only looked at the tests, so you can ignore my comments for now :) http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py File Lib/test/test_scandir.py (right): http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:12: except ImportError: This check is not necessary for stdlib. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:21: if IS_PY3: You can remove this, too. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:50: os.mkdir(TEST_PATH) You could use ``test.support.TESTFN`` here. See https://docs.python.org/3.5/library/test.html for documentation. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:104: if not hasattr(unittest.TestCase, 'skipTest'): You can remove this, too. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:117: entries = dict((e.name, e) for e in self.scandir_func(TEST_PATH)) entries = {e.name: e for e in self.scandir_func(TEST_PATH)} http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:131: os_stat = os.stat(os.path.join(TEST_PATH, entry.name)) It would be great to use the new subTest() feature here: https://docs.python.org/3.5/library/unittest.html#distinguishing-test-iterati... http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:142: assert hasattr(entry, 'name') Please use assertTrue. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:177: return self.skipTest('symbolic links not supported') This could be written as a decorator: @unittest.skipUnless(symlinks_supported, 'symbolic links not supported') http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:242: self.assertTrue(isinstance(entry.name, str)) assertIsInstance http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:253: self.scandir_func = scandir.scandir_generic Can this be a class attribute instead? class TestScandirGeneric(TestMixin, unittest.TestCase): scandir = scandir.scandir_generic has_file_attributes = False http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:258: if hasattr(scandir, 'scandir_python'): You can take a look PEP 399 at http://legacy.python.org/dev/peps/pep-0399/ for testing both Python and C implementations. http://bugs.python.org/review/22524/diff/13005/Lib/test/test_scandir.py#newco... Lib/test/test_scandir.py:274: if hasattr(os, 'scandir'): You could remove this check, too.
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16365: static char *_follow_symlinks_keywords[] = {"follow_symlinks", NULL}; no need to use a "_" prefix, the variable is local (static keyword). An alternative is to write an helper function to factorize the code. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16370: PyObject *path; I would prefer to store the "scandir_path" (directory) and join the path on demand using a read-only property, to reduce the memory footprint. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16390: #if defined(MS_WINDOWS) && !defined(HAVE_OPENDIR) Why not using FindFirstFile if opendir() is available? FindFirstFile provides more data (avoid more syscalls) no? It may avoid the need of your "typedef ... mode_t". http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16392: typedef unsigned short mode_t; It's not a good idea to reuse names (mode_t) which are used in system headers on other platforms. If you really need a "portable" type, use a "Py_" prefix, or a different name. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16401: DirEntry_do_stat(DirEntry *self, int follow_symlinks) I suggest the name: DirEntry_get_stat(). IMO two functions, stat and lstat, would make the code simpler. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16412: DEFAULT_DIR_FD, follow_symlinks); You can pass 1 instead of follow_symlinks here, to be more explicit. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16413: path_cleanup(&path); I would prefer an explicit error check here (self->stat). http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16417: self->lstat = _pystat_fromstructstat(&self->win32_lstat); I would prefer an explicit error check here. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16419: Py_XINCREF(self->lstat); So Py_XINCREF can be replaced with Py_INCREF. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16519: DirEntry_is_dir_file(DirEntry *self, int follow_symlinks, mode_t mode_bits) I suggest the name: DirEntry_test_file_type() or DirEntry_test_mode(). http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16532: if (self->d_type == DT_UNKNOWN || (follow_symlinks && is_symlink)) { I don't like #ifdef/#else which both start a "{" block. I prefer to use a variable (ex: need_stat) and move the if out of the #ifdef. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16540: Py_RETURN_FALSE; Oh, this behaviour was not defined explicitly in the PEP. I don't like ignoring errors. I would prefer to raise an error. os.path.isdir() is different than DirEntry, because DirEntry objects are supposed to have a short lifetime and files are not supposed to disappear. We should probably discuss this point on python-dev. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16544: st_mode = PyObject_GetAttrString(stat, "st_mode"); You can call Py_CLEAR(stat) here. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16545: if (!st_mode) { style: i prefer without {...} for such simple statement. (Check maybe in the PEP 7.) http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16549: mode = PyLong_AsLong(st_mode); You have to check for errors: if (mode == -1 && PyErr_Occurred()) goto error; http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16555: result = 0; I don't understand why you don't check: result = (mode_bits == S_IFLNK). Is it because DirEntry_is_symlink() cals DirEntry_is_dir_file() with follow_links=0? Add maybe an assertion (mode_bits != S_IFLNK) to make it more obvious? Or add a comment? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16557: else { I guess that mode_bits can only be S_IFDIR or S_IFREG at this point? If yes, please add an assertion (or maybe a comment) to make it more obvious (and avoid bugs). http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16564: result = (mode_bits == S_IFDIR) ? self->d_type == DT_DIR : style: I prefer an explicit if() instead of the ternary form... but it's up to you ;-) http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16607: "this entry's filename, relative to scandir()'s \"path\" argument"}, In the PEP, you wrote "the entry's filename". It's maybe better than "this entry's filename"? Or "The filename of the entry, ..."? I'm not the best to write documentation, I'm not an english native speaker. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16663: _join_path_filenameA(char *path_narrow, char* filename, Py_ssize_t filename_len) no need for "_" prefix. the second parameter may be called "name" instead of "filename", to stay consistent with DirEntry? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16669: path_narrow = "."; Does it mean that you build a path like "./name"? Why not returning "name"? I don't know if it's safe to have a special case for path_narrow == ".". On Mac OS 9, the current directory is ":", but this OS is no more supported since Python 2.4: http://legacy.python.org/dev/peps/pep-0011/ (but the macpath module still exists in Python 3.5, I don't know why.) http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16675: path_len = strlen(path_narrow); hum? this second call looks like a mistake. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16677: if (filename_len == -1) { style: no need for {...} http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16682: result = PyMem_Malloc(path_len + filename_len + 2); you can write it: path_len + 1 + filename_len + 1, so IMO the comment becomes useless. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16688: if (path_len > 0) { I don't understand how path_len can be negative. If path_len is 0, why not adding the filename? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16690: #if defined(MS_WINDOWS) && !defined(HAVE_OPENDIR) ntpath.join() is very complex. This code should be carefully reviewed. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16692: result[path_len++] = '\\'; nitpick: you may check before calling PyMem_Malloc: (add_sep = /*...*/;), to compute exactly the length of the result. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16697: strcpy(result + path_len, filename); filename_len is not used there. I guess that it's just an optimization? Is it possible on UNIX that filename doesn't end with a NUL byte? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16732: _join_path_filenameW(wchar_t *path_wide, wchar_t* filename) same remaks than _join_path_filenameA ;) http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16761: make_DirEntry(path_t *path, void *data) the classic naming theme is: DirEntry_new.
Sign in to reply to this message.
I've updated the patch per Victor's code review at http://bugs.python.org/issue22524 (see scandir-2.patch) http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16370: PyObject *path; > I would prefer to store the "scandir_path" (directory) and join the path on > demand using a read-only property, to reduce the memory footprint. Could do. How do you do read-only properties in C? Part of the problem is it doesn't really save much memory (assuming the scandir_path is long and the name is short, which is probably the case most of the time), so I think the benefit would outweigh the cost. Unless there's some way to have only one copy of scandir_path ... but because the DirEntry objects can live longer than the scandir iterator, I'm not sure how you'd do that. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16390: #if defined(MS_WINDOWS) && !defined(HAVE_OPENDIR) > Why not using FindFirstFile if opendir() is available? FindFirstFile provides > more data (avoid more syscalls) no? It may avoid the need of your "typedef ... > mode_t". True. I copied this from the existing posix_listdir() implementation, but it doesn't really apply here. I'll fix. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16392: typedef unsigned short mode_t; On 2014/10/07 23:28:49, haypo wrote: > It's not a good idea to reuse names (mode_t) which are used in system headers on > other platforms. If you really need a "portable" type, use a "Py_" prefix, or a > different name. Fixed. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16401: DirEntry_do_stat(DirEntry *self, int follow_symlinks) On 2014/10/07 23:28:49, haypo wrote: > I suggest the name: DirEntry_get_stat(). > > IMO two functions, stat and lstat, would make the code simpler. Fair point, fixed. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16413: path_cleanup(&path); On 2014/10/07 23:28:49, haypo wrote: > I would prefer an explicit error check here (self->stat). I don't understand why this is needed. You need to cleanup the path in either case, and if posix_do_stat() returns NULL (error), then DirEntry_do_stat() will as well. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16417: self->lstat = _pystat_fromstructstat(&self->win32_lstat); On 2014/10/07 23:28:49, haypo wrote: > I would prefer an explicit error check here. Some as above -- the error/NULL will simply bubble up. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16419: Py_XINCREF(self->lstat); On 2014/10/07 23:28:49, haypo wrote: > So Py_XINCREF can be replaced with Py_INCREF. Hmmm, but is this an advantage? It's just makes the code more complicated? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16519: DirEntry_is_dir_file(DirEntry *self, int follow_symlinks, mode_t mode_bits) On 2014/10/07 23:28:49, haypo wrote: > I suggest the name: DirEntry_test_file_type() or DirEntry_test_mode(). Done. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16532: if (self->d_type == DT_UNKNOWN || (follow_symlinks && is_symlink)) { On 2014/10/07 23:28:49, haypo wrote: > I don't like #ifdef/#else which both start a "{" block. I prefer to use a > variable (ex: need_stat) and move the if out of the #ifdef. Good call, done. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16540: Py_RETURN_FALSE; On 2014/10/07 23:28:49, haypo wrote: > Oh, this behaviour was not defined explicitly in the PEP. I don't like ignoring > errors. I would prefer to raise an error. > > os.path.isdir() is different than DirEntry, because DirEntry objects are > supposed to have a short lifetime and files are not supposed to disappear. > > We should probably discuss this point on python-dev. It may not be in the PEP, but we did discuss it on python-dev -- it was decided that seeing the DirEntry is_dir/is_file/is_symlink methods were based on the (more modern?) pathlib API, DirEntry should do the same thing as scandir. Which in this case is to return False if the file doesn't exist, or raise for all other errors. This is documented in the scandir docs in os.rst. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16544: st_mode = PyObject_GetAttrString(stat, "st_mode"); On 2014/10/07 23:28:49, haypo wrote: > You can call Py_CLEAR(stat) here. I don't really understand why you'd use Py_CLEAR(stat) over here Py_DECREF here. I'm already calling Py_DECREF in the error and non-error cases. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16545: if (!st_mode) { On 2014/10/07 23:28:49, haypo wrote: > style: i prefer without {...} for such simple statement. (Check maybe in the PEP > 7.) I've used consistent always-include-the-braces style throughout. There's a mixture for single-statement if's within posixmodule.c, and PEP 7 doesn't mandate one way or the other. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16549: mode = PyLong_AsLong(st_mode); Done. But where is it documented that PyLong_AsLong returns -1 on error? http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16555: result = 0; On 2014/10/07 23:28:49, haypo wrote: > I don't understand why you don't check: result = (mode_bits == S_IFLNK). Is it > because DirEntry_is_symlink() cals DirEntry_is_dir_file() with follow_links=0? > Add maybe an assertion (mode_bits != S_IFLNK) to make it more obvious? Or add a > comment? Yeah, that's right. Added an assert. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16564: result = (mode_bits == S_IFDIR) ? self->d_type == DT_DIR : On 2014/10/07 23:28:49, haypo wrote: > style: I prefer an explicit if() instead of the ternary form... but it's up to > you ;-) Done. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16669: path_narrow = "."; On 2014/10/07 23:28:49, haypo wrote: > Does it mean that you build a path like "./name"? Why not returning "name"? I > don't know if it's safe to have a special case for path_narrow == ".". No, this happens when you call scandir() with no arguments, the scandir path defaults to ".". In this case the DirEntry.path attributes return './' + name (or '.\\' + name on Windows), which I think is as expected, and implicit in the docs in the default arg for scandir(path='.') http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16675: path_len = strlen(path_narrow); On 2014/10/07 23:28:49, haypo wrote: > hum? this second call looks like a mistake. You're dead right. Fixed. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16688: if (path_len > 0) { On 2014/10/07 23:28:49, haypo wrote: > I don't understand how path_len can be negative. If path_len is 0, why not > adding the filename? I don't see how path_len can be negative either. I copied this from _listdir_windows_no_opendir in posixmodule.c, but I think it's bogus. It could be zero at this point in the code, but in that case it doesn't matter as you say. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16690: #if defined(MS_WINDOWS) && !defined(HAVE_OPENDIR) On 2014/10/07 23:28:49, haypo wrote: > ntpath.join() is very complex. This code should be carefully reviewed. I think this is okay. This isn't an actual os.path.join(). It's also copied directly from _listdir_windows_no_opendir in posixmodule.c http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16692: result[path_len++] = '\\'; On 2014/10/07 23:28:49, haypo wrote: > nitpick: you may check before calling PyMem_Malloc: (add_sep = /*...*/;), to > compute exactly the length of the result. It might save 1 byte in some cases, so I'd rather keep the code simple (and the same as the similar code in listdir). http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16697: strcpy(result + path_len, filename); On 2014/10/07 23:28:49, haypo wrote: > filename_len is not used there. I guess that it's just an optimization? Is it > possible on UNIX that filename doesn't end with a NUL byte? Yeah, just an optimization -- if we know it, we pass it in to join_path_filenameA(). I believe all filenames on Linux have to end with NUL, because they're char* without a length in the kernel APIs. http://bugs.python.org/review/22524/diff/13005/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16761: make_DirEntry(path_t *path, void *data) On 2014/10/07 23:28:49, haypo wrote: > the classic naming theme is: DirEntry_new. Fixed.
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13913/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1058 Lib/os.py:1058: dir_it = _scandir(path) Oh, this variable is useless. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1138 Lib/os.py:1138: dir_it = _scandir(fsencode(path)) Ditto, this variable is useless.
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13913/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode993 Lib/os.py:993: if name == 'nt': Some methods are common in both implementations. So it may be worth to write this as: class DirEntry: # docstring # common methods if name == 'nt': # nt methods else: # posix methods http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1002 Lib/os.py:1002: self._stat = None For performance you can avoid initializing self._stat and like in __init__. Instead you can initialize class attributes _stat and like at compile time. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1007 Lib/os.py:1007: if self._inode is None: I didn't test, but may be following rewriting slightly speed up repeated access to the attribute: inode = self._inode if inode is None: ... inode = self._inode = ... return inode And the same for other cached methods. If the call of stat() is much slower than raising and catching exception, it may be better use different idiom: try: return self._inode # _inode shouldn't be initialized to None except AttributeError: ... self._inode = lstat.st_ino return self._inode http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1008 Lib/os.py:1008: lstat = stat(self.path, follow_symlinks=False) lstat() may be a little faster. I don't know if it is significant. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1058 Lib/os.py:1058: dir_it = _scandir(path) Shouldn't _scandir() itself check argument type? http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1090 Lib/os.py:1090: if self._lstat is None: self._lstat always is not None after calling self.is_symlink(). http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1100 Lib/os.py:1100: if (self._d_type == DT_UNKNOWN or May be use None instead of DT_UNKNOWN? It looks more Pythonic and unlike to DT_UNKNOWN is present on all systems. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1141 Lib/os.py:1141: name = name.decode(encoding, 'surrogateescape') Why not fsdecode? http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16585: closedir(scandir->dirp); It would be safer first set scandir->dirp to NULL and then call closedir() with saved value of scandir->dirp. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16596: scandir_close(scandir); May be raise ResourceWarning if scandir is not closed yet? http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16607: #ifdef MS_WINDOWS Almost all code is surrounded with #ifdef MS_WINDOWS. May be make one big #ifdef MS_WINDOWS for all function? http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16617: static char *keywords[] = {"path", NULL}; I think keyword parameter is not needed in this private function. It complicates source code and slows down executing. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16625: path = PyUnicode_AsWideCharString(pathobj, &path_len); May be needed to check if patch contains the null character. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16629: if (!PyArg_ParseTupleAndKeywords(args, kwds, "y#:_scandir", keywords, "y#" is not safe. Use "y*" instead. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16640: filepath = (wchar_t *)PyMem_Malloc(sizeof(wchar_t) * (path_len + 5)); Please add a comment what 5 means (len("/*.*") + 1). And may be use PyMem_Realloc()? http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16658: && filepath[path_len-1] != '\\' && filepath[path_len-1] != '/') { '\\'? http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16660: filepath[path_len+1] = '\0'; If path_len is 0 or filepath ends with '/', ending null isn't copied. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16748: PyErr_SetNone(PyExc_StopIteration); What will happen when use exhausted scandir iterator? it = _scandir(...) for x in it: pass for x in it: pass http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16792: return Py_BuildValue("Nll", filename, ep->d_ino, ep->d_type); d_type is not present on all systems. Check _DIRENT_HAVE_D_TYPE. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16798: "Scandir", /* tp_name */ I suggest to make module name mandatory (see issue20204).
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13913/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode993 Lib/os.py:993: if name == 'nt': On 2015/02/12 14:05:17, storchaka wrote: > Some methods are common in both implementations. So it may be worth to write > this as: > > class DirEntry: > # docstring > # common methods > if name == 'nt': > # nt methods > else: > # posix methods Done. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1002 Lib/os.py:1002: self._stat = None On 2015/02/12 14:05:17, storchaka wrote: > For performance you can avoid initializing self._stat and like in __init__. > Instead you can initialize class attributes _stat and like at compile time. This optimization is incompatible with the __slots__ optimization. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1007 Lib/os.py:1007: if self._inode is None: On 2015/02/12 14:05:17, storchaka wrote: > I didn't test, but may be following rewriting slightly speed up repeated access > to the attribute: > > inode = self._inode > if inode is None: > ... > inode = self._inode = ... > return inode > > And the same for other cached methods. > > If the call of stat() is much slower than raising and catching exception, it may > be better use different idiom: > > try: > return self._inode # _inode shouldn't be initialized to None > except AttributeError: > ... > self._inode = lstat.st_ino > return self._inode I prefer to stay avoid from hardcore optimizations in DirEntry. I prefer to keep simple code. The main optimization comes from the reduction of system calls. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1008 Lib/os.py:1008: lstat = stat(self.path, follow_symlinks=False) On 2015/02/12 14:05:17, storchaka wrote: > lstat() may be a little faster. I don't know if it is significant. That's surprising. os.stat(follow_symlinks=False) and os.lstat() are supposed to call the same C function, lstat(). It's probably the overhead of parsing function arguments. Again, I prefer to keep the code simple and easier to read, than having the fastest performances. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1058 Lib/os.py:1058: dir_it = _scandir(path) On 2015/02/12 14:05:17, storchaka wrote: > Shouldn't _scandir() itself check argument type? Oh, it was already the case. This check is useless. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1090 Lib/os.py:1090: if self._lstat is None: On 2015/02/12 14:05:17, storchaka wrote: > self._lstat always is not None after calling self.is_symlink(). I merged Windows and POSIX implementation together, so such micro-optimization is no more possible. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1100 Lib/os.py:1100: if (self._d_type == DT_UNKNOWN or On 2015/02/12 14:05:17, storchaka wrote: > May be use None instead of DT_UNKNOWN? It looks more Pythonic and unlike to > DT_UNKNOWN is present on all systems. Hum, I prefer to keep os._scandir() as a light wrapper to the C function. So I prefer DT_UNKNOWN. The attribute is not public. http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1141 Lib/os.py:1141: name = name.decode(encoding, 'surrogateescape') On 2015/02/12 14:05:17, storchaka wrote: > Why not fsdecode? Ok, this line is a micro-optimization :-) On POSIX, fsencode() is well defined, so I consider that it's ok to inline it here. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16585: closedir(scandir->dirp); On 2015/02/12 14:05:17, storchaka wrote: > It would be safer first set scandir->dirp to NULL and then call closedir() with > saved value of scandir->dirp. Done. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16596: scandir_close(scandir); On 2015/02/12 14:05:17, storchaka wrote: > May be raise ResourceWarning if scandir is not closed yet? The Scandir object is an iterator. The common way for use an iterator is "for item in it: ...", there is no explicit call to a close() method. By the way, Scandir has no close() method. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16607: #ifdef MS_WINDOWS On 2015/02/12 14:05:17, storchaka wrote: > Almost all code is surrounded with #ifdef MS_WINDOWS. May be make one big #ifdef > MS_WINDOWS for all function? Done. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16617: static char *keywords[] = {"path", NULL}; On 2015/02/12 14:05:17, storchaka wrote: > I think keyword parameter is not needed in this private function. It complicates > source code and slows down executing. Done. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16625: path = PyUnicode_AsWideCharString(pathobj, &path_len); On 2015/02/12 14:05:17, storchaka wrote: > May be needed to check if patch contains the null character. Done. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16629: if (!PyArg_ParseTupleAndKeywords(args, kwds, "y#:_scandir", keywords, On 2015/02/12 14:05:17, storchaka wrote: > "y#" is not safe. Use "y*" instead. Crap. I was not sure if it was safe or not. The function copies immediatly the string. But ok, let's avoid unsafe formats. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16640: filepath = (wchar_t *)PyMem_Malloc(sizeof(wchar_t) * (path_len + 5)); On 2015/02/12 14:05:17, storchaka wrote: > Please add a comment what 5 means (len("/*.*") + 1). > > And may be use PyMem_Realloc()? Why realloc? To release 5 bytes? It sounds completly overkill. Most memory allocator rounds the size to 8 bytes anyway. 5 bytes is very small. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16658: && filepath[path_len-1] != '\\' && filepath[path_len-1] != '/') { On 2015/02/12 14:05:17, storchaka wrote: > '\\'? Oh, it comes from Ben's code. I agree that it's unexpected on POSIX. I don't see code testing \ in posixpath, so I drop it from here. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16660: filepath[path_len+1] = '\0'; On 2015/02/12 14:05:17, storchaka wrote: > If path_len is 0 or filepath ends with '/', ending null isn't copied. It was already copied by memcpy(). Hum, I replaced memcpy() with strcpy(), no need for overkill optimization here. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16748: PyErr_SetNone(PyExc_StopIteration); On 2015/02/12 14:05:17, storchaka wrote: > What will happen when use exhausted scandir iterator? > > it = _scandir(...) > for x in it: > pass > for x in it: > pass I modified the code to close the iterator when StopIteration is closed. I also added a closed attribute to raise immediatly StopIteration if the iterator is closed. And I added a unit test to test this border case. It should never occur since os._scandir() is private, and scandir() doesn't consume _scandir() twice. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16792: return Py_BuildValue("Nll", filename, ep->d_ino, ep->d_type); On 2015/02/12 14:05:17, storchaka wrote: > d_type is not present on all systems. Check _DIRENT_HAVE_D_TYPE. I modified it to: #if defined(__GLIBC__) && !defined(_DIRENT_HAVE_D_TYPE) d_type = DT_UNKNOWN; #else d_type = ep->d_type; #endif return Py_BuildValue("Nll", filename, ep->d_ino, d_type); I prefer to be optimistic and consider that all libc always define it. If not, we will fit it. I just checked: d_type is available on Linux, FreeBSD and OpenBSD. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16798: "Scandir", /* tp_name */ On 2015/02/12 14:05:17, storchaka wrote: > I suggest to make module name mandatory (see issue20204). Done.
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13913/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1008 Lib/os.py:1008: lstat = stat(self.path, follow_symlinks=False) On 2015/02/12 15:41:48, haypo wrote: > On 2015/02/12 14:05:17, storchaka wrote: > > lstat() may be a little faster. I don't know if it is significant. > > That's surprising. os.stat(follow_symlinks=False) and os.lstat() are supposed to > call the same C function, lstat(). It's probably the overhead of parsing > function arguments. > > Again, I prefer to keep the code simple and easier to read, than having the > fastest performances. But Posix version uses lstat(). http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1100 Lib/os.py:1100: if (self._d_type == DT_UNKNOWN or On 2015/02/12 15:41:48, haypo wrote: > On 2015/02/12 14:05:17, storchaka wrote: > > May be use None instead of DT_UNKNOWN? It looks more Pythonic and unlike to > > DT_UNKNOWN is present on all systems. > > Hum, I prefer to keep os._scandir() as a light wrapper to the C function. So I > prefer DT_UNKNOWN. The attribute is not public. How than you will handle the case when DT_UNKNOWN is not defined? The None is good placement value. And see dir_fd-supporting functions. They don't use platform specific integer constant AT_FDCWD, but use None. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16640: filepath = (wchar_t *)PyMem_Malloc(sizeof(wchar_t) * (path_len + 5)); On 2015/02/12 15:41:48, haypo wrote: > On 2015/02/12 14:05:17, storchaka wrote: > > Please add a comment what 5 means (len("/*.*") + 1). > > > > And may be use PyMem_Realloc()? > > Why realloc? To release 5 bytes? It sounds completly overkill. Most memory > allocator rounds the size to 8 bytes anyway. 5 bytes is very small. First for simplicity. No need to copy string explicitly. And second for performance. If there are free 5 bytes at the and of the block, nor memory allocation, nor copying would be not needed. http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16660: filepath[path_len+1] = '\0'; On 2015/02/12 15:41:48, haypo wrote: > On 2015/02/12 14:05:17, storchaka wrote: > > If path_len is 0 or filepath ends with '/', ending null isn't copied. > > It was already copied by memcpy(). Hum, I replaced memcpy() with strcpy(), no > need for overkill optimization here. Ah, I missed "+ 1" in memcpy(). http://bugs.python.org/review/22524/diff/13913/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16792: return Py_BuildValue("Nll", filename, ep->d_ino, ep->d_type); On 2015/02/12 15:41:48, haypo wrote: > On 2015/02/12 14:05:17, storchaka wrote: > > d_type is not present on all systems. Check _DIRENT_HAVE_D_TYPE. > > I modified it to: > > #if defined(__GLIBC__) && !defined(_DIRENT_HAVE_D_TYPE) > d_type = DT_UNKNOWN; > #else > d_type = ep->d_type; > #endif > return Py_BuildValue("Nll", filename, ep->d_ino, d_type); > > I prefer to be optimistic and consider that all libc always define it. If not, > we will fit it. > > I just checked: d_type is available on Linux, FreeBSD and OpenBSD. But DT_UNKNOWN will likely not defined if _DIRENT_HAVE_D_TYPE is not defined, I suggest following code: #ifdef _DIRENT_HAVE_D_TYPE return Py_BuildValue("Nli", filename, (long)ep->d_ino, ep->d_type); #else return Py_BuildValue("NlO", filename, (long)ep->d_ino, Py_None); #endif
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13913/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1008 Lib/os.py:1008: lstat = stat(self.path, follow_symlinks=False) On 2015/02/12 16:11:39, storchaka wrote: > On 2015/02/12 15:41:48, haypo wrote: > > On 2015/02/12 14:05:17, storchaka wrote: > > > lstat() may be a little faster. I don't know if it is significant. > > > > That's surprising. os.stat(follow_symlinks=False) and os.lstat() are supposed > to > > call the same C function, lstat(). It's probably the overhead of parsing > > function arguments. > > > > Again, I prefer to keep the code simple and easier to read, than having the > > fastest performances. > > But Posix version uses lstat(). timeit: os.lstat('.') takes 1.49 usec, os.stat('.', follow_symlinks=False) takes 2.53 usec. It's the same syscall (lstat). Hum, parsing (keyword) arguments has a important cost compared to syscalls :-( Ok to use lstat(). I didn't notice the the code was already using lstat(). http://bugs.python.org/review/22524/diff/13913/Lib/os.py#newcode1100 Lib/os.py:1100: if (self._d_type == DT_UNKNOWN or On 2015/02/12 16:11:39, storchaka wrote: > On 2015/02/12 15:41:48, haypo wrote: > > On 2015/02/12 14:05:17, storchaka wrote: > > > May be use None instead of DT_UNKNOWN? It looks more Pythonic and unlike to > > > DT_UNKNOWN is present on all systems. > > > > Hum, I prefer to keep os._scandir() as a light wrapper to the C function. So I > > prefer DT_UNKNOWN. The attribute is not public. > > How than you will handle the case when DT_UNKNOWN is not defined? The None is > good placement value. > > And see dir_fd-supporting functions. They don't use platform specific integer > constant AT_FDCWD, but use None. Ok. I changed my patch to use None.
Sign in to reply to this message.
https://bugs.python.org/review/22524/diff/13917/Lib/os.py File Lib/os.py (right): https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1025 Lib/os.py:1025: if name == 'nt' and self._inode is None: self._inode is not None on Posix, so "name == 'nt'" is redundant. https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1032 Lib/os.py:1032: def stat(self, follow_symlinks=True): Should follow_symlinks be keyword-only parameter? https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1118 Lib/os.py:1118: I think DirEntry isn't expected to be pickleable. So needed __getstate__() that raises an exception. And the test for this. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py File Lib/test/test_os.py (right): https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2714 Lib/test/test_os.py:2714: entries = dict((entry.name, entry) You can use dict comprehension here. {entry.name: entry for entry in os.scandir(self.path)} https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2732 Lib/test/test_os.py:2732: def check_entry(self, entry, name, is_dir, is_file, is_symlink): is_dir and is_file are not used. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2760 Lib/test/test_os.py:2760: link = hasattr(os, 'link') I would make separate tests for links. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2812 Lib/test/test_os.py:2812: try: with support.change_cwd(self.path): https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2862 Lib/test/test_os.py:2862: entry.stat(follow_symlinks=False) I would first test with follow_symlinks=False. stat(follow_symlinks=True) affects following stat(follow_symlinks=False). https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2869 Lib/test/test_os.py:2869: if not support.can_symlink(): @support.skip_unless_symlink https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2926 Lib/test/test_os.py:2926: Could you add a test for non-directory and non-regular file? And may be for hidden files on Windows and for files starting with a dot or two. And for argument ending with sep or altsep. https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c File Modules/posixmodule.c (right): https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c#newcode... Modules/posixmodule.c:16685: /* +5: len("/") + null byte */ +2 https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c#newcode... Modules/posixmodule.c:16761: is_finished = 1; break? https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c#newcode... Modules/posixmodule.c:16839: #if defined(__GLIBC__) && !defined(_DIRENT_HAVE_D_TYPE) I would write this simpler: #if defined(_DIRENT_HAVE_D_TYPE) || defined(DT_UNKNOWN) if (ep->d_type != DT_UNKNOWN) return Py_BuildValue("yli", ep->d_name, (long)ep->d_ino, ep->d_type); else #else return Py_BuildValue("ylO", ep->d_name, (long)ep->d_ino, Py_None); #endif
Sign in to reply to this message.
https://bugs.python.org/review/22524/diff/13917/Lib/os.py File Lib/os.py (right): https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1025 Lib/os.py:1025: if name == 'nt' and self._inode is None: On 2015/02/12 20:20:30, storchaka wrote: > self._inode is not None on Posix, so "name == 'nt'" is redundant. Right. https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1032 Lib/os.py:1032: def stat(self, follow_symlinks=True): On 2015/02/12 20:20:30, storchaka wrote: > Should follow_symlinks be keyword-only parameter? Ah yes. If we follow os.stat(): it must be a keyword-only parameter. https://bugs.python.org/review/22524/diff/13917/Lib/os.py#newcode1118 Lib/os.py:1118: On 2015/02/12 20:20:30, storchaka wrote: > I think DirEntry isn't expected to be pickleable. So needed __getstate__() that > raises an exception. And the test for this. I don't see why you would pickle a DirEntry object. See the doc: "DirEntry instances are not intended to be stored in long-lived data structures" https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py File Lib/test/test_os.py (right): https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2714 Lib/test/test_os.py:2714: entries = dict((entry.name, entry) On 2015/02/12 20:20:30, storchaka wrote: > You can use dict comprehension here. > > {entry.name: entry for entry in os.scandir(self.path)} Ah yes. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2732 Lib/test/test_os.py:2732: def check_entry(self, entry, name, is_dir, is_file, is_symlink): On 2015/02/12 20:20:30, storchaka wrote: > is_dir and is_file are not used. Ah yes, I now rely on os.stat() to ensure that os.stat() and entry methods are consistent. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2760 Lib/test/test_os.py:2760: link = hasattr(os, 'link') On 2015/02/12 20:20:30, storchaka wrote: > I would make separate tests for links. Agreed. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2862 Lib/test/test_os.py:2862: entry.stat(follow_symlinks=False) On 2015/02/12 20:20:30, storchaka wrote: > I would first test with follow_symlinks=False. stat(follow_symlinks=True) > affects following stat(follow_symlinks=False). If a method modify the entry, it would be better to generate a new entry for each check. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2926 Lib/test/test_os.py:2926: On 2015/02/12 20:20:30, storchaka wrote: > Could you add a test for non-directory and non-regular file? Which kind of file? How can I create such non-regular file? > And may be for hidden files on Windows and for files starting with a dot or two. I don't know how to create an hidden file on Windows. Filename starting with a dot looks to be an useful test. https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c File Modules/posixmodule.c (right): https://bugs.python.org/review/22524/diff/13917/Modules/posixmodule.c#newcode... Modules/posixmodule.c:16685: /* +5: len("/") + null byte */ On 2015/02/12 20:20:30, storchaka wrote: > +2 oops
Sign in to reply to this message.
I was NOT reviewing for correctness; I was thinking of Paul Moore's comment that he assumed it was basic C code, and wouldn't require platform expertise. I'm not sure that is a safe assumption; the platform-specific parts are mostly constants and part of the known API differences, but ... those API differences may well be a problem for someone without strong #ifdef blinders. Putting it in a separate file (so that it won't be there for everyone looking at posixmodule for other reasons) might be helpful. http://bugs.python.org/review/22524/diff/13104/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13104/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16373: #ifdef MS_WINDOWS Does this platform-specific #ifdef within a struct definition cause problems for pickling? If so, is that a problem, or just something to document in a footnote? (The docs do say they aren't intended for long-term storage, but pickle is sometimes used for communications within a cluster... though it is hard to come up with a use case that isn't probably already misguided.) http://bugs.python.org/review/22524/diff/13104/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16891: #endif Same question -- does this block pickling, and if so, what is the minimal level of warning to put in the docs?
Sign in to reply to this message.
https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py File Lib/test/test_os.py (right): https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2862 Lib/test/test_os.py:2862: entry.stat(follow_symlinks=False) On 2015/02/13 15:14:14, haypo wrote: > On 2015/02/12 20:20:30, storchaka wrote: > > I would first test with follow_symlinks=False. stat(follow_symlinks=True) > > affects following stat(follow_symlinks=False). > > If a method modify the entry, it would be better to generate a new entry for > each check. Agreed. https://bugs.python.org/review/22524/diff/13917/Lib/test/test_os.py#newcode2926 Lib/test/test_os.py:2926: On 2015/02/13 15:14:14, haypo wrote: > Which kind of file? How can I create such non-regular file? os.mkfifo(), os.mknode(). > I don't know how to create an hidden file on Windows. Looks as there are no other ways besides using ctypes, adding new test function in _testcapi or using third-part module such as win32api.
Sign in to reply to this message.
http://bugs.python.org/review/22524/diff/13104/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/13104/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16373: #ifdef MS_WINDOWS On 2015/02/13 17:15:47, Jim.J.Jewett wrote: > Does this platform-specific #ifdef within a struct definition cause problems for > pickling? If so, is that a problem, or just something to document in a > footnote? (The docs do say they aren't intended for long-term storage, but > pickle is sometimes used for communications within a cluster... though it is > hard to come up with a use case that isn't probably already misguided.) I think that DirEntry shouldn't be pickleable
Sign in to reply to this message.
First review of the C code. I didn't read the doc or tests. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16450: __int64 win32_file_index; I'm surprised that the type is signed: nFileIndexHigh and nFileIndexLow fields of BY_HANDLE_FILE_INFORMATION are unsigned (DWORD). But since Py_stat_struct.st_ino already uses the type __int64, it's better to discuss this in a separate issue. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16451: int got_file_index; If I remember correctly, file index cannot be zero on Windows. You may use 0 to check if the file index is known or not. But I'm not sure, so you may keep got_file_index. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16468: } ScandirIterator; Maybe you should move ScandirIterator structor below, after DirEntry code, to move it closer to the code using it. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16480: /* Forward reference to make it easier to implement DirEntry_is_symlink etc */ Just say "Forward reference", it's enough ;-) http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16490: if (self->d_type != DT_UNKNOWN) { If the body has only one line, you can avoid { and } characters. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16506: if (!path_converter(self->path, &path)) { No need for { } when the body has a single line. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16532: if (follow_symlinks) { To not put almost the whole function in an if block, you can invert the test: if (!follow_symlinks) return DirEntry_get_lstat(self); ... http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16550: if (!po_is_symlink) { Having to handle NULL and to call PyObject_IsTrue() is annoying. You should split is_symlink() in two parts: a C version which returns an int (0 or 1), and a Python wrapper which simply calls PyBool_FromLong(). So you can reuse it for the Windows path a few line above, and it will probably break the dependency cycle. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16592: int result = 0; It's strange that you have to initialize result here. It's always set, no? http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16595: unsigned long dir_bits; Not needed on POSIX. Only declare dir_bits on Windows: put it into #ifdef MS_WINDOWS. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16625: Py_DECREF(stat); Even if the code is currently ok, it's safer to use Py_CLEAR(), so it's still safe to call "goto error". http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16705: path_cleanup(&path); You don't need the path_t type here: wchar_t *path = PyUnicode_AsUnicode(self->path); if (path == NULL) return NULL; if (win32_lstat_w(path.wide, &stat) != 0) { Py_DECREF(path); return PyErr_SetExcFromWindowsErrWithFilenameObject(PyExc_OSError, 0, self->path); } Py_DECREF(path); http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16722: return PyUnicode_FromFormat("<" MODNAME ".DirEntry %R>", self->name); Hum, you should just use "<DirEntry %r>", drop the module name. And it can be surprising to see "nt" or "posix" module name. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16803: result = PyMem_Malloc((path_len + 1 + wcslen(filename) + 1) * sizeof(wchar_t)); You should use PyMem_New() to detect integer overflow. Example: size = path_len + 1 + wcslen(filename) + 1; result = PyMem_New(wchar_t, size); The new size variable is required, because PyMem_New() is a macro which duplicates parameters. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16812: result[path_len++] = SEP; ntpath.join('c:', 'name) returns 'c:name'. I'm surprised, I expected "c:\name". I didn't know that "c:name" is valid. I should try scandir("c:") on Windows. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16820: DirEntry_new(path_t *path, WIN32_FIND_DATAW *dataW) I would prefer a "From" name: DirEntry_FromFindData() for example. "new" is usually used for regular constructor taking variable parameters. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16837: entry->name = PyUnicode_FromWideChar(dataW->cFileName, wcslen(dataW->cFileName)); You can just pass -1 for the length. (FromWideChar calls wcslen() for you.) http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16858: Py_XDECREF(entry); Py_DECREF() is ok here. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16867: You shouldn't call FindClose() with INVALID_HANDLE_VALUE. Add: if (iterator->handle == INVALID_HANDLE_VALUE) return 1; http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16874: iterator->handle = INVALID_HANDLE_VALUE; Hum, you should set "iterator->handle = INVALID_HANDLE_VALUE;" even if FindClose() failed. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16897: ScandirIterator_close(iterator); I'm not sure that it's ok call FindClose() here. You may retry calling next() later, no? By the way, ScandirIterator_close(iterator) can replace the Windows last error (SetLastError). http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16912: iterator->first_time = 0; It the first file is not "." or "..", next() always return the same file. You should move this line before the previous if. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16926: join_path_filenameA(char *path_narrow, char* filename, Py_ssize_t filename_len) I hate these W/A suffix. Please avoid them on POSIX! Rename the function to join_path_filename(), or just join_path(). http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16944: result = PyMem_Malloc(path_len + 1 + filename_len + 1); Ditto: use PyMem_New(). http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16978: if (!path->narrow || !PyBytes_Check(path->object)) { This is just wrong: you must always decode the name from the filesystem encoding. I don't understand this test. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17004: int result; You should not call closedir() with NULL. Add: if (iteratator->dirp == NULL) return 1; http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17009: if (result != 0) { I'm not sure that you do something useful if FindClose/closedir failed. You may just ignore errors :-/ Especially when ScandirIterator_close() is called from dealloc. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17038: ScandirIterator_close(iterator); Ditto: don't close the iterator on error. You may retry on error. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17070: #endif Please mention the related #ifdef, I guess: #endif /* MS_WINDOWS */ http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17076: path_cleanup(&iterator->path); IMO it's very important to close the iterator here! http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17142: "os.scandir() doesn't support bytes path on Windows, use Unicode instead"); You should use a more generic message: "str expected, got %s" % Py_TYPE(iterator->path.object)->tp_name Hum, I don't remember if I wrote a test with other types like int and float. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17156: PyMem_Free(path_strW); /* We're done with path_strW now */ IMO the comment is useless. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17165: iterator->dirp = opendir(iterator->path.narrow ? iterator->path.narrow : "."); I would prefer to store the name in a variable, it would ease debug. Example: if (iterator->path.narrow) name = iterator->path.narrow; else name = "."; Py_BEGIN_ALLOW_THREADS terator->dirp = opendir(name); Py_END_ALLOW_THREADS I regulary debug such C code with gdb.
Sign in to reply to this message.
Thanks -- comments below. I've made these changes and will upload scandir-8.patch in the next few minutes. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c File Modules/posixmodule.c (right): http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16450: __int64 win32_file_index; On 2015/02/27 16:33:00, haypo wrote: > I'm surprised that the type is signed: nFileIndexHigh and nFileIndexLow fields > of BY_HANDLE_FILE_INFORMATION are unsigned (DWORD). But since > Py_stat_struct.st_ino already uses the type __int64, it's better to discuss this > in a separate issue. Yes, this surprised me too, but I just copied what Py_stat_struct.st_ino did, so agreed if it's an issue it should be a separate issue. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16451: int got_file_index; On 2015/02/27 16:33:00, haypo wrote: > If I remember correctly, file index cannot be zero on Windows. You may use 0 to > check if the file index is known or not. But I'm not sure, so you may keep > got_file_index. I thought about that, and it's *probably* okay, but I'm pretty hesitant to do that, as there's nothing at all about nFileIndex not being zero in the MS documentation: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363788(v=vs.85).aspx http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16468: } ScandirIterator; On 2015/02/27 16:33:00, haypo wrote: > Maybe you should move ScandirIterator structor below, after DirEntry code, to > move it closer to the code using it. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16480: /* Forward reference to make it easier to implement DirEntry_is_symlink etc */ On 2015/02/27 16:33:00, haypo wrote: > Just say "Forward reference", it's enough ;-) Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16490: if (self->d_type != DT_UNKNOWN) { On 2015/02/27 16:33:00, haypo wrote: > If the body has only one line, you can avoid { and } characters. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16532: if (follow_symlinks) { On 2015/02/27 16:33:00, haypo wrote: > To not put almost the whole function in an if block, you can invert the test: > > if (!follow_symlinks) > return DirEntry_get_lstat(self); > > ... Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16550: if (!po_is_symlink) { On 2015/02/27 16:33:00, haypo wrote: > Having to handle NULL and to call PyObject_IsTrue() is annoying. You should > split is_symlink() in two parts: a C version which returns an int (0 or 1), and > a Python wrapper which simply calls PyBool_FromLong(). > > So you can reuse it for the Windows path a few line above, and it will probably > break the dependency cycle. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16592: int result = 0; On 2015/02/27 16:33:00, haypo wrote: > It's strange that you have to initialize result here. It's always set, no? Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16595: unsigned long dir_bits; On 2015/02/27 16:33:00, haypo wrote: > Not needed on POSIX. Only declare dir_bits on Windows: put it into #ifdef > MS_WINDOWS. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16625: Py_DECREF(stat); On 2015/02/27 16:33:00, haypo wrote: > Even if the code is currently ok, it's safer to use Py_CLEAR(), so it's still > safe to call "goto error". Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16705: path_cleanup(&path); On 2015/02/27 16:33:00, haypo wrote: > You don't need the path_t type here: > > wchar_t *path = PyUnicode_AsUnicode(self->path); > if (path == NULL) > return NULL; > > if (win32_lstat_w(path.wide, &stat) != 0) { > Py_DECREF(path); > return PyErr_SetExcFromWindowsErrWithFilenameObject(PyExc_OSError, > 0, self->path); > } > Py_DECREF(path); Thanks -- good call. Done. Though I don't think you want Py_DECREF called on path, because PyUnicode_AsUnicode returns a read-only pointer, doesn't create a new object. It's also a wchar_t, not a PyObject. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16722: return PyUnicode_FromFormat("<" MODNAME ".DirEntry %R>", self->name); On 2015/02/27 16:33:00, haypo wrote: > Hum, you should just use "<DirEntry %r>", drop the module name. And it can be > surprising to see "nt" or "posix" module name. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16803: result = PyMem_Malloc((path_len + 1 + wcslen(filename) + 1) * sizeof(wchar_t)); On 2015/02/27 16:33:00, haypo wrote: > You should use PyMem_New() to detect integer overflow. Example: > > size = path_len + 1 + wcslen(filename) + 1; > result = PyMem_New(wchar_t, size); > > The new size variable is required, because PyMem_New() is a macro which > duplicates parameters. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16812: result[path_len++] = SEP; On 2015/02/27 16:33:00, haypo wrote: > ntpath.join('c:', 'name) returns 'c:name'. I'm surprised, I expected "c:\name". > I didn't know that "c:name" is valid. I should try scandir("c:") on Windows. Hmmm, that's kinda weird. However, I copied this from os.listdir(), which does exactly the same thing, so I'm loathe to change it in case there's a good reason. Can open a separate issue if needed. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16820: DirEntry_new(path_t *path, WIN32_FIND_DATAW *dataW) On 2015/02/27 16:33:00, haypo wrote: > I would prefer a "From" name: DirEntry_FromFindData() for example. "new" is > usually used for regular constructor taking variable parameters. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16837: entry->name = PyUnicode_FromWideChar(dataW->cFileName, wcslen(dataW->cFileName)); On 2015/02/27 16:33:00, haypo wrote: > You can just pass -1 for the length. (FromWideChar calls wcslen() for you.) Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16858: Py_XDECREF(entry); On 2015/02/27 16:33:00, haypo wrote: > Py_DECREF() is ok here. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16867: On 2015/02/27 16:33:00, haypo wrote: > You shouldn't call FindClose() with INVALID_HANDLE_VALUE. Add: > > if (iterator->handle == INVALID_HANDLE_VALUE) > return 1; Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16874: iterator->handle = INVALID_HANDLE_VALUE; On 2015/02/27 16:33:00, haypo wrote: > Hum, you should set "iterator->handle = INVALID_HANDLE_VALUE;" even if > FindClose() failed. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16897: ScandirIterator_close(iterator); On 2015/02/27 16:33:00, haypo wrote: > I'm not sure that it's ok call FindClose() here. You may retry calling next() > later, no? > > By the way, ScandirIterator_close(iterator) can replace the Windows last error > (SetLastError). Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16912: iterator->first_time = 0; On 2015/02/27 16:33:00, haypo wrote: > It the first file is not "." or "..", next() always return the same file. You > should move this line before the previous if. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16926: join_path_filenameA(char *path_narrow, char* filename, Py_ssize_t filename_len) On 2015/02/27 16:33:00, haypo wrote: > I hate these W/A suffix. Please avoid them on POSIX! Rename the function to > join_path_filename(), or just join_path(). I hate them too, but I think this is helpful to distinguish, given there's a join_path_filenameW for Windows. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16944: result = PyMem_Malloc(path_len + 1 + filename_len + 1); On 2015/02/27 16:33:00, haypo wrote: > Ditto: use PyMem_New(). Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:16978: if (!path->narrow || !PyBytes_Check(path->object)) { On 2015/02/27 16:33:00, haypo wrote: > This is just wrong: you must always decode the name from the filesystem > encoding. I don't understand this test. I think this is actually correct. I mimicked the behaviour of os.listdir() here. What's happening is there are 3 ways to call scandir: no args (which is equivalent to '.'), unicode arg, bytes arg: 1) scandir(): path.narrow=NULL -> unicode 2) scandir('unicode'): path.narrow=char*, path.object=u'unicode' -> unicode 3) scandir(b'bytes'): path.narrow=char*, path.object=b'bytes' -> bytes So this sets name/path to unicode in cases 1 and 2, but to bytes in case 3. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17004: int result; On 2015/02/27 16:33:00, haypo wrote: > You should not call closedir() with NULL. Add: > > if (iteratator->dirp == NULL) > return 1; Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17009: if (result != 0) { On 2015/02/27 16:33:00, haypo wrote: > I'm not sure that you do something useful if FindClose/closedir failed. You may > just ignore errors :-/ Especially when ScandirIterator_close() is called from > dealloc. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17038: ScandirIterator_close(iterator); On 2015/02/27 16:33:00, haypo wrote: > Ditto: don't close the iterator on error. You may retry on error. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17070: #endif On 2015/02/27 16:33:00, haypo wrote: > Please mention the related #ifdef, I guess: #endif /* MS_WINDOWS */ That feels weird to me, as it's actually ending the POSIX #else block, or not? http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17076: path_cleanup(&iterator->path); On 2015/02/27 16:33:00, haypo wrote: > IMO it's very important to close the iterator here! Done, thanks! http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17142: "os.scandir() doesn't support bytes path on Windows, use Unicode instead"); On 2015/02/27 16:33:00, haypo wrote: > You should use a more generic message: > > "str expected, got %s" % Py_TYPE(iterator->path.object)->tp_name > > Hum, I don't remember if I wrote a test with other types like int and float. I don't quite understand why. My error message is much more specific and informative; I'd expect the interpreter to raise a generic message if it didn't know any better, but I think specific code can be more helpful. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17156: PyMem_Free(path_strW); /* We're done with path_strW now */ On 2015/02/27 16:33:00, haypo wrote: > IMO the comment is useless. Done. http://bugs.python.org/review/22524/diff/14036/Modules/posixmodule.c#newcode1... Modules/posixmodule.c:17165: iterator->dirp = opendir(iterator->path.narrow ? iterator->path.narrow : "."); On 2015/02/27 16:33:00, haypo wrote: > I would prefer to store the name in a variable, it would ease debug. Example: > > if (iterator->path.narrow) > name = iterator->path.narrow; > else > name = "."; > > Py_BEGIN_ALLOW_THREADS > terator->dirp = opendir(name); > Py_END_ALLOW_THREADS > > I regulary debug such C code with gdb. Done.
Sign in to reply to this message.
|
