bpo-32798: Add restriction on the offset parameter for mmap.flush in the docs #5621

pablogsal · 2018-02-11T15:21:46Z

https://bugs.python.org/issue32798

berkerpeksag · 2018-02-12T20:57:41Z

Doc/library/mmap.rst

      changes to the given range of bytes will be flushed to disk; otherwise, the
-      whole extent of the mapping is flushed.
+      whole extent of the mapping is flushed. *offset* must be a multiple of the
+      PAGESIZE.


Since this is the platform independent documentation, I think it should say "PAGESIZE or ALLOCATIONGRANULARITY".

berkerpeksag · 2018-02-12T20:58:11Z

Doc/library/mmap.rst

      the object is destroyed.  If *offset* and *size* are specified, only
      changes to the given range of bytes will be flushed to disk; otherwise, the
-      whole extent of the mapping is flushed.
+      whole extent of the mapping is flushed. *offset* must be a multiple of the


Style nit: Please follow the present style and start the sentence with two spaces.

bedevere-bot · 2018-02-12T20:59:36Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2018-02-12T21:03:29Z

I have made the requested changes; please review again

bedevere-bot · 2018-02-12T21:03:31Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

serhiy-storchaka · 2018-03-27T14:51:34Z

Doc/library/mmap.rst

Please format these constants as

:const:`PAGESIZE` and :const:`ALLOCATIONGRANULARITY`

And in the constructor too.

pablogsal · 2018-03-27T21:28:59Z

I have made the requested changes; please review again

bedevere-bot · 2018-03-27T21:29:01Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

pablogsal · 2018-03-27T21:29:18Z

CC: @serhiy-storchaka

serhiy-storchaka · 2018-03-28T09:47:54Z

Doc/library/mmap.rst

Please update also the formatting of other mention of these constants.

zhangyangyu · 2018-03-28T12:48:39Z

FYI, see my comment in the issue, this seems not a necessity required by POSIX. Not sure it matters.

pablogsal · 2018-03-28T14:42:51Z

@zhangyangyu @serhiy-storchaka Should we close the PR then?

zhangyangyu · 2018-03-28T15:07:58Z

I am not sure since there is already such requirement in the doc, like mmap.mmap. Maybe it's already the practical standard? Adding it seems won't hurt.

berkerpeksag · 2018-08-22T17:30:19Z

Doc/library/mmap.rst

-   defaults to 0.  *offset* must be a multiple of the PAGESIZE or
-   ALLOCATIONGRANULARITY.
+   defaults to 0.  *offset* must be a multiple of the :const:`PAGESIZE` or
+   :const:`ALLOCATIONGRANULARITY`.


ALLOCATIONGRANULARITY is equivalent of PAGESIZE under Unix. Since this documentation for the Unix version of mmap.mmap(), perhapd we can remove ALLOCATIONGRANULARITY from the documentation?

Nit: "a multiple of the"

Can "the" be removed here?

Done in 2d90d92

I think it is better to use the same condition on all platforms. Since ALLOCATIONGRANULARITY is equivalent of PAGESIZE under Unix, you can use ALLOCATIONGRANULARITY as well as PAGESIZE.

Maybe change the wording to something like: "offset must be a multiple of ALLOCATIONGRANULARITY which is equal to PAGESIZE under Unix"?

serhiy-storchaka · 2018-08-24T13:27:18Z

And please update also the formatting of other mention of ALLOCATIONGRANULARITY.

bedevere-bot · 2018-10-20T00:37:57Z

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2018-10-20T00:37:57Z

Thanks @pablogsal for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2018-10-20T00:38:08Z

GH-9989 is a backport of this pull request to the 3.7 branch.

…the docs (pythonGH-5621) Add restriction on the offset parameter for mmap.flush. Explain that ALLOCATIONGRANULARITY is the same as PAGESIZE in Unix. (cherry picked from commit 027664a) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

vstinner · 2018-10-20T00:38:12Z

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

Aaaaaaaaaaaaaaaaaaaaaaaaaaaaah.

bedevere-bot · 2018-10-20T00:38:16Z

GH-9990 is a backport of this pull request to the 3.6 branch.

…the docs (pythonGH-5621) Add restriction on the offset parameter for mmap.flush. Explain that ALLOCATIONGRANULARITY is the same as PAGESIZE in Unix. (cherry picked from commit 027664a) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

miss-islington · 2018-10-20T00:39:45Z

Thanks @pablogsal for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

bedevere-bot · 2018-10-20T00:39:54Z

GH-9991 is a backport of this pull request to the 2.7 branch.

…the docs (pythonGH-5621) Add restriction on the offset parameter for mmap.flush. Explain that ALLOCATIONGRANULARITY is the same as PAGESIZE in Unix. (cherry picked from commit 027664a) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

…the docs (GH-5621) Add restriction on the offset parameter for mmap.flush. Explain that ALLOCATIONGRANULARITY is the same as PAGESIZE in Unix. (cherry picked from commit 027664a) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

serhiy-storchaka · 2018-10-20T05:49:52Z

I left this issue open because its author is a core developer now, and is able to merge his PRs yourself (after making last moment changes if needed).

Add restriction on the offset parameter for mmap.flush

a478cb0

the-knights-who-say-ni added the CLA signed label Feb 11, 2018

bedevere-bot added the awaiting review label Feb 11, 2018

berkerpeksag added the skip news label Feb 12, 2018

berkerpeksag requested changes Feb 12, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 12, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 12, 2018

berkerpeksag added needs backport to 3.6 labels Feb 12, 2018

berkerpeksag approved these changes Feb 12, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 12, 2018

serhiy-storchaka reviewed Mar 27, 2018

View reviewed changes

pablogsal force-pushed the bpo32798 branch from 7c0a07a to bef7628 Compare March 27, 2018 21:28

bedevere-bot added awaiting change review and removed awaiting merge labels Mar 27, 2018

serhiy-storchaka reviewed Mar 28, 2018

View reviewed changes

Doc/library/mmap.rst Outdated

Copy link

Member

serhiy-storchaka Mar 28, 2018

Choose a reason for hiding this comment

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

Please update also the formatting of other mention of these constants.

Correct style

8a65d8a

pablogsal force-pushed the bpo32798 branch from bef7628 to 8a65d8a Compare March 28, 2018 22:55

berkerpeksag reviewed Aug 22, 2018

View reviewed changes

fixup! Correct style

2d90d92

pablogsal added 2 commits August 24, 2018 17:20

Add formatting for ALLOCATIONGRANULARITY occurrences

f334ea4

Explain that ALLOCATIONGRANULARITY is the same as PAGESIZE in Unix

39488c5

serhiy-storchaka approved these changes Aug 27, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 27, 2018

vstinner merged commit 027664a into python:master Oct 20, 2018

bedevere-bot removed the awaiting merge label Oct 20, 2018

bedevere-bot removed the needs backport to 3.7 label Oct 20, 2018

bedevere-bot removed the needs backport to 3.6 label Oct 20, 2018

vstinner added the needs backport to 2.7 label Oct 20, 2018

bedevere-bot removed the needs backport to 2.7 label Oct 20, 2018

pablogsal deleted the bpo32798 branch May 19, 2021 18:57

Uh oh!

bpo-32798: Add restriction on the offset parameter for mmap.flush in the docs #5621

bpo-32798: Add restriction on the offset parameter for mmap.flush in the docs #5621

Uh oh!

Conversation

pablogsal commented Feb 11, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 12, 2018

Uh oh!

pablogsal commented Feb 12, 2018

Uh oh!

bedevere-bot commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Mar 27, 2018

Uh oh!

bedevere-bot commented Mar 27, 2018

Uh oh!

pablogsal commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu commented Mar 28, 2018

Uh oh!

pablogsal commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhangyangyu commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 24, 2018

Uh oh!

bedevere-bot commented Oct 20, 2018

Uh oh!

miss-islington commented Oct 20, 2018

Uh oh!

bedevere-bot commented Oct 20, 2018

Uh oh!

vstinner commented Oct 20, 2018

Uh oh!

bedevere-bot commented Oct 20, 2018

Uh oh!

miss-islington commented Oct 20, 2018

Uh oh!

bedevere-bot commented Oct 20, 2018

Uh oh!

serhiy-storchaka commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pablogsal commented Feb 11, 2018 •

edited by bedevere-bot

Loading

pablogsal commented Mar 28, 2018 •

edited

Loading