homepage

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.

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

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

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

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

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

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

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

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

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?

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

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

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

Didn't you forget to attach the patch?

It seems I did. Attached now for real :)

Patch looks good to me, thank you :)

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 

Patch committed.
Thanks for the report and the patch!