classification
Title: mmap.read requires an argument
Type: enhancement Stage: resolved
Components: Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: jcea, jcon, nadeem.vawda, neologix, petri.lehtinen, pitrou, python-dev, rich-noir
Priority: normal Keywords: patch

Created on 2011-05-06 19:54 by rich-noir, last changed 2012-10-29 23:58 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
mmap_read_all.patch petri.lehtinen, 2011-05-27 10:37 Impementation and tests review
mmap_read_all_2.patch petri.lehtinen, 2011-05-27 10:54 Implementation, tests & docs review
mmap_read_all_3.patch petri.lehtinen, 2011-05-30 18:46 Implementation, tests & docs review
mmap_read_all_4.patch petri.lehtinen, 2011-06-07 18:03 review
Messages (18)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2011-06-06 18:48
Didn't you forget to attach the patch?
msg137873 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-06-07 18:03
It seems I did. Attached now for real :)
msg137914 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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) * (Python committer) Date: 2011-06-08 18:13
Patch committed.
Thanks for the report and the patch!
History
Date User Action Args
2012-10-29 23:58:19jceasetnosy: + jcea
2012-10-29 16:31:15r.david.murraylinkissue16358 superseder
2011-06-08 18:13:29neologixsetstatus: open -> closed
resolution: fixed
messages: + msg137920

stage: patch review -> resolved
2011-06-08 17:18:36python-devsetnosy: + python-dev
messages: + msg137917
2011-06-08 15:52:30pitrousetmessages: + msg137914
2011-06-07 18:03:55petri.lehtinensetfiles: + mmap_read_all_4.patch

messages: + msg137873
2011-06-06 18:48:39pitrousetmessages: + msg137765
2011-06-06 18:29:11petri.lehtinensetmessages: + msg137763
2011-06-06 14:56:05pitrousetmessages: + msg137740
2011-06-06 14:39:16neologixsetmessages: + msg137739
2011-06-05 18:24:48petri.lehtinensetmessages: + msg137706
2011-06-05 11:22:51pitrousetmessages: + msg137689
2011-05-31 17:04:27neologixsetmessages: + msg137370
2011-05-31 08:46:36petri.lehtinensetmessages: + msg137346
2011-05-30 18:46:00petri.lehtinensetfiles: + mmap_read_all_3.patch

messages: + msg137318
2011-05-30 18:02:05neologixsetnosy: + neologix

messages: + msg137315
stage: needs patch -> patch review
2011-05-27 10:54:22petri.lehtinensetfiles: + mmap_read_all_2.patch

messages: + msg137043
2011-05-27 10:37:29petri.lehtinensetfiles: + mmap_read_all.patch

nosy: + petri.lehtinen
messages: + msg137042

keywords: + patch
2011-05-07 07:05:24jconsetnosy: + jcon
2011-05-06 20:13:34nadeem.vawdasetnosy: + nadeem.vawda
2011-05-06 19:56:51pitrousetversions: - Python 2.6, Python 3.1, Python 2.7, Python 3.2
nosy: + pitrou

messages: + msg135364

type: behavior -> enhancement
stage: needs patch
2011-05-06 19:54:13rich-noircreate