Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mmap.read requires an argument #56230

Closed
rich-noir mannequin opened this issue May 6, 2011 · 18 comments
Closed

mmap.read requires an argument #56230

rich-noir mannequin opened this issue May 6, 2011 · 18 comments
Labels
type-feature A feature request or enhancement

Comments

@rich-noir
Copy link
Mannequin

rich-noir mannequin commented May 6, 2011

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
    Copy link
    Mannequin Author

    rich-noir mannequin commented May 6, 2011

    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.

    @rich-noir rich-noir mannequin added the type-bug An unexpected behavior, bug, or error label May 6, 2011
    @pitrou
    Copy link
    Member

    pitrou commented May 6, 2011

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

    @pitrou pitrou added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels May 6, 2011
    @akheron
    Copy link
    Member

    akheron commented May 27, 2011

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

    @akheron
    Copy link
    Member

    akheron commented May 27, 2011

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

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 30, 2011

    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
    Copy link
    Member

    akheron commented May 30, 2011

    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
    Copy link
    Member

    akheron commented May 31, 2011

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

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 31, 2011

    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
    Copy link
    Member

    pitrou commented Jun 5, 2011

    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
    Copy link
    Member

    akheron commented Jun 5, 2011

    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
    Copy link
    Mannequin

    neologix mannequin commented Jun 6, 2011

    > 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
    Copy link
    Member

    pitrou commented Jun 6, 2011

    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
    Copy link
    Member

    akheron commented Jun 6, 2011

    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
    Copy link
    Member

    pitrou commented Jun 6, 2011

    Didn't you forget to attach the patch?

    @akheron
    Copy link
    Member

    akheron commented Jun 7, 2011

    It seems I did. Attached now for real :)

    @pitrou
    Copy link
    Member

    pitrou commented Jun 8, 2011

    Patch looks good to me, thank you :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2011

    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
    Copy link
    Mannequin

    neologix mannequin commented Jun 8, 2011

    Patch committed.
    Thanks for the report and the patch!

    @neologix neologix mannequin closed this as completed Jun 8, 2011
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @akheron @pitrou