Skip to content

bpo-2122: Make mmap.flush() behave same on all platforms#8692

Merged
berkerpeksag merged 7 commits intopython:masterfrom
berkerpeksag:2122-mmap-flush
Aug 22, 2018
Merged

bpo-2122: Make mmap.flush() behave same on all platforms#8692
berkerpeksag merged 7 commits intopython:masterfrom
berkerpeksag:2122-mmap-flush

Conversation

@berkerpeksag
Copy link
Copy Markdown
Member

@berkerpeksag berkerpeksag commented Aug 6, 2018 •
edited by bedevere-bot
Loading

@berkerpeksag berkerpeksag requested a review from zooba August 8, 2018 08:27
@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Aug 15, 2018

Is it possible to test a failed mmap.flush()?

Since this is a behavior breaking change, please add a note in What's New (section "Changes in the Python API").

@berkerpeksag
Copy link
Copy Markdown
Member Author

berkerpeksag commented Aug 21, 2018

Is it possible to test a failed mmap.flush()?

It's possible to get EINVAL under Unix, but I don't know about Windows. I've just pushed a test and let's see what happens.

Since this is a behavior breaking change, please add a note in What's New (section "Changes in the Python API").

Done.

@berkerpeksag
Copy link
Copy Markdown
Member Author

berkerpeksag commented Aug 21, 2018

Since we already do range checking in mmap_flush_method(), I don't think it's easy to trigger an error under Windows.

serhiy-storchaka
serhiy-storchaka approved these changes Aug 22, 2018
View reviewed changes
Comment thread Doc/whatsnew/3.8.rst
can also be retrieved using the :meth:`~asyncio.Task.get_name` method.

* The :meth:`mmap.flush() <mmap.mmap.flush>` method now returns ``None`` on
success and raises an exception on error under all platforms. Previously,
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to accent the main reason of this change. "Previously, the behavior was platform-depended: on success, a nonzero value was returned under Windows and a zero value was returned under Unix, on error, zero was returned under Windows and an exception was raised under Unix." Maybe native speakers can suggest better wording. In any case this PR LGTM.

Copy link
Copy Markdown
Member Author

@berkerpeksag berkerpeksag Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I just tweaked it a bit.

@berkerpeksag berkerpeksag merged commit e7d4b2f into python:master Aug 22, 2018
@berkerpeksag berkerpeksag deleted the 2122-mmap-flush branch August 22, 2018 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@zooba zooba Awaiting requested review from zooba

@zware zware Awaiting requested review from zware

@briancurtin briancurtin Awaiting requested review from briancurtin

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@berkerpeksag @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot