Message2973

The javadocs for FileChannel.map say that the mapped buffer isn't tied to the channel that created it, so that even if the channel is closed, the buffer is still available.  Why not just require that the fileno that comes in be an instance of FileChannel, since that's what we return for fileno in instances of files?  Changing the type of object passed into mmap.mmap makes this module different enough from the CPython version that sharing a name with it is probably more harmful than good.  Definitely any CPython code that uses mmap will have to be special cased to work with Jython.

I don't understand why PyMmap needs to extend PyString.  The mmap type in CPython is not an instance of str.  However in Jython, extending PyString and keeping a String copy of the mmapped data breaks the general use case of mmapping.  From my understanding, when you mmap a file you're expecting the OS to map it into memory as you're reading it, and then to page it out as parts of it are no longer being read.  Since this patch reads the whole file into a Java String, it's actually far worse than reading the file into a single byte array and holding onto it: it'll use twice the memory of the file for chars instead of bytes.  This difference is going to make mmap useless for anyone mapping a large file.  Where all is it expected to be an actual instance of str?  I see re.search checking that the pattern is a str, but not the object to be searched.

What do you mean "a doubt about implementing the buffer interface"?  I'm not sure what the buffer interface is.

Your check for os.name starting with "Lin" as a check for unix is a pretty bad idea since there are several unix variants out there that have a JVM that don't start with "Lin".  Is there any reason to actually support an os distinction on Jython's version of mmap?  Since it's the same across all platforms, why not just use the access keyword and raise errors for flag, prot and tagname?  Platform independent mmapping has to use access already.

I'd really like to avoid using internal Sun packages in Jython if at all possible: if we use them, we can't run on all JVMs.  Is there any reason for the close method to actually "close" the mmaped buffer?  Why not just null out the reference to it and let the garbage collector get it when it will?  Then you can just check for the buffer being null on any operation on the mmap type and raise an exception if it happens.

These are the major issues I noticed in a quick check of the patch.  There may be smaller issues turned up by a closer examination, but I figured it was a good idea to get these out of the way before doing that.