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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Aug 22, 2018
Merged

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

merged 7 commits into from
Aug 22, 2018

Conversation

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

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
Member Author

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
Member Author

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
Doc/whatsnew/3.8.rst
@@ -265,6 +265,13 @@ Changes in the Python API
task name is visible in the ``repr()`` output of :class:`asyncio.Task` and
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
Member

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
Member Author

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