| msg135362 - (view) |
Author: K Richard Pixley (rich-noir) |
Date: 2011-05-06 19:54 |
| mmap.read requires a argument. Since most file-like objects do not, this breaks the file-like object illusion. mmap.read argument should be optional, presumably defaulting to the entire mmap'd area. |
|
|
| msg135364 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2011-05-06 19:56 |
| Sounds like a reasonable request, although I don't think it's a bug fix in itself (there are probably other places where mmap breaks the file-like expectation). |
|
|
| msg137042 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-05-27 10:37 |
| Added a patch. It was only a matter of making the size parameter optional. |
|
|
| msg137043 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-05-27 10:54 |
| Updated the patch to also update the documentation of mmap.read(). |
|
|
| msg137315 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-05-30 18:02 |
| The patch looks good to me. In your test, you don't explicitely close the mmap object: it's not a problem with CPython since it will get unmmapped, and the file descriptor - if it's a file-backed mapping - will get closed, as soon as it gets out of scope, but it would be cleaner to close it explicitely with something like self.addCleanup(m.close). |
|
|
| msg137318 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-05-30 18:46 |
| Thanks for the comments. I attached a new patch that uses self.addCleanup(m.close), and also adds a versionchanged directive to the docs. |
|
|
| msg137346 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-05-31 08:46 |
| I noticed that RawIOBase.read, TextIOBase.read, etc also accept None as the argument, treating it as -1. Should this be implemented, too? |
|
|
| msg137370 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-05-31 17:04 |
| > I noticed that RawIOBase.read, TextIOBase.read, etc also accept None as the > argument, treating it as -1. Should this be implemented, too? > That's because of the _PyIO_ConvertSsize_t converter, which silently converts None to -1. There's probably a good reason for doing this in the _io module, but I don't see any reason to do the same thing in the mmap module (apart from being consistent, of course). Antoine? If the patch is fine as-is, I'd like to commit it. |
|
|
| msg137689 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-05 11:22 |
| > That's because of the _PyIO_ConvertSsize_t converter, which silently > converts None to -1. > There's probably a good reason for doing this in the _io module I'm not sure about the original reason, but I find None as the default "omitted" value prettier than -1 myself, so I think it's a good thing :) |
|
|
| msg137706 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-06-05 18:24 |
| Antoine Pitrou wrote: > I'm not sure about the original reason, but I find None as the default "omitted" value prettier than -1 myself, so I think it's a good thing :) Yeah, and it's also good for consistency with other file-like objects. Can I use _PyIO_ConvertSsize_t? Or should I duplicate its functionality in mmapmodule.c? |
|
|
| msg137739 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-06-06 14:39 |
| >> That's because of the _PyIO_ConvertSsize_t converter, which silently >> converts None to -1. >> There's probably a good reason for doing this in the _io module > > I'm not sure about the original reason, but I find None as the default "omitted" value prettier than -1 myself, so I think it's a good thing :) > If being pretty is the only reason for this choice, then I think that documenting the method as method:: read([n]) is simpler and cleaner . But you've got much more experience than me, so I won't argue any further :-) > Can I use _PyIO_ConvertSsize_t? Or should I duplicate its > functionality in mmapmodule.c? I let Antoine answer that. |
|
|
| msg137740 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-06 14:56 |
| > If being pretty is the only reason for this choice, then I think that > documenting the method as > > method:: read([n]) > > is simpler and cleaner . > > But you've got much more experience than me, so I won't argue any further :-) There are contexts where it is easier to give the default argument than call the method without argument, though, and that's where I find None more intuitive than -1 :) Arguably it's not very important, though. > > Can I use _PyIO_ConvertSsize_t? Or should I duplicate its > > functionality in mmapmodule.c? > > I let Antoine answer that. I'm not sure. This would require including iomodule.h, which is supposed to be private to the _io module. I think duplicating it is fine, since the code is probably simple anyway (I'm too lazy to take a look right now :-)). |
|
|
| msg137763 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-06-06 18:29 |
| > I think duplicating it is fine, since the code is probably simple anyway Added a new patch. I duplicated the None converting logic in _io/_iomodule.c to mmapmodule.c, changed the doc to say "read([n])", and expanded the test cases a bit. |
|
|
| msg137765 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-06 18:48 |
| Didn't you forget to attach the patch? |
|
|
| msg137873 - (view) |
Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2011-06-07 18:03 |
| It seems I did. Attached now for real :) |
|
|
| msg137914 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-08 15:52 |
| Patch looks good to me, thank you :) |
|
|
| msg137917 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-06-08 17:18 |
| New changeset 964d0d65a2a9 by Charles-François Natali in branch 'default': Issue #12021: Make mmap's read() method argument optional. Patch by Petri http://hg.python.org/cpython/rev/964d0d65a2a9 |
|
|
| msg137920 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2011-06-08 18:13 |
| Patch committed. Thanks for the report and the patch! |
|
|