mmap.read requires an argument

[rich-noir]

BPO	12021
Nosy	@jcea, @pitrou, @akheron
Files	mmap_read_all.patch: Impementation and tests mmap_read_all_2.patch: Implementation, tests & docs mmap_read_all_3.patch: Implementation, tests & docs mmap_read_all_4.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2011-06-08.18:13:29.036>
created_at = <Date 2011-05-06.19:54:13.505>
labels = ['type-feature']
title = 'mmap.read requires an argument'
updated_at = <Date 2012-10-29.23:58:19.285>
user = 'https://bugs.python.org/rich-noir'

bugs.python.org fields:

activity = <Date 2012-10-29.23:58:19.285>
actor = 'jcea'
assignee = 'none'
closed = True
closed_date = <Date 2011-06-08.18:13:29.036>
closer = 'neologix'
components = []
creation = <Date 2011-05-06.19:54:13.505>
creator = 'rich-noir'
dependencies = []
files = ['22142', '22143', '22199', '22275']
hgrepos = []
issue_num = 12021
keywords = ['patch']
message_count = 18.0
messages = ['135362', '135364', '137042', '137043', '137315', '137318', '137346', '137370', '137689', '137706', '137739', '137740', '137763', '137765', '137873', '137914', '137917', '137920']
nosy_count = 8.0
nosy_names = ['jcea', 'pitrou', 'nadeem.vawda', 'neologix', 'python-dev', 'jcon', 'rich-noir', 'petri.lehtinen']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue12021'
versions = ['Python 3.3']

[rich-noir]

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.

[pitrou]

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).

[akheron]

Added a patch. It was only a matter of making the size parameter optional.

[akheron]

Updated the patch to also update the documentation of mmap.read().

[neologix]

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).

[akheron]

Thanks for the comments. I attached a new patch that uses self.addCleanup(m.close), and also adds a versionchanged directive to the docs.

[akheron]

I noticed that RawIOBase.read, TextIOBase.read, etc also accept None as the argument, treating it as -1. Should this be implemented, too?

[neologix]

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.

[pitrou]

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 :)

[akheron]

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?

[neologix]

> 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.

[pitrou]

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 :-)).

[akheron]

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.

[pitrou]

Didn't you forget to attach the patch?

[akheron]

It seems I did. Attached now for real :)

[pitrou]

Patch looks good to me, thank you :)

[python-dev]

New changeset 964d0d65a2a9 by Charles-François Natali in branch 'default':
Issue bpo-12021: Make mmap's read() method argument optional. Patch by Petri
http://hg.python.org/cpython/rev/964d0d65a2a9

[neologix]

Patch committed.
Thanks for the report and the patch!