Skip to content

Conversation

@EasyIsrael
Copy link

added windows support

added windows support
Copy link
Owner

@vandry vandry left a comment

Choose a reason for hiding this comment

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

It looks like the mmap documentation says you can use the form with access= as a keyword argument for all operating systems. I tested; it works. So you don't need an if statement at all. See https://docs.python.org/2/library/mmap.html

self.raw = mmap.mmap(fd, fullsize, mmap.MAP_SHARED, mmap.PROT_READ)

if (os.name == "posix"):
#Linux-Version:
Copy link
Owner

Choose a reason for hiding this comment

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

There is nothing Linux specific about this. It'll work on, say, Solaris or BSD as well.
Actually, you want probably just do without this comment altogether. It's clear what the "if" is doing.

Copy link
Author

Choose a reason for hiding this comment

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

That's right. You can throw the comments away

self.headerlen = headerlen
self.raw = mmap.mmap(fd, fullsize, mmap.MAP_SHARED, mmap.PROT_READ)

if (os.name == "posix"):
Copy link
Owner

Choose a reason for hiding this comment

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

Style nit: don't use parentheses here.

self.raw = mmap.mmap(fd, fullsize, mmap.MAP_SHARED, mmap.PROT_READ)
else:
#Windows-Version:
self.raw = mmap.mmap(fd, fullsize, access=mmap.ACCESS_READ)#mmap.MAP_SHARED, mmap.PROT_READ
Copy link
Owner

Choose a reason for hiding this comment

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

Please get rid of the commented text at the end of the line.

@EasyIsrael
Copy link
Author

I've just tested it under windows. The parameter access=ACCESS_READ is important to not throw an error. If you sucessfully tested it under posix- system, then it's fine. Then it will be just one line intead of three.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants