Alan,
Webrev has been updated accordingly at
http://cr.openjdk.java.net/~sherman/7130915/webrev
with changes
(1) added a CFStringCreateMutable(...) != null check in both io and nio
native, though it is
unlikely to fail here because we are passing a NULL and 0 length,
like new StringBuilder()
invocation, if it fails the system probably goes very wrong. Both
FStringAppendCharacters
and CFStringNormalize are void return type.
(2) The first line of path.toCharArray in normalizeJavaPath is a typo,
it is supposed to be
deleted. The implementation only goes toCharArray when it needs to
go native. Currently
it uses >0x80 as the fast path criteria, it is possible to expose
some sun.text.normalizer's
internal methods to have a "quick nfc" check, but I'm not sure how
much the performance
gain would be, but might worth some investigation later.
(3) Comments have been added for those override-able methods in
UnixFileSystem.
(4) blank lines have been removed from dispatcher.c
(5) I don't think we need to do the HFS check, given we are only doing
nfc/nfd stuff now, as
long as it is a MacOSX, I believe its nfc/nfd layer will be there.
Copyright has been re-copy/
pasted and we now only use only bugid.
-Sherman
On 6/26/12 8:02 AM, Alan Bateman wrote:
On 26/06/2012 07:00, Xueming Shen wrote:
On 6/25/12 10:58 PM, Xueming Shen wrote:
Hi,
While I still believe that case-insensitive is the right choice for
File/Path on MacOSX, it is
suggested that we might want to be a little conservative in this
patch, with the assumption
that this patch will be backport to 7u release after being baked in
jdk8 for a while (given
Apple's JDK6 is case sensitive for File, it might be too much for a
update releases to go
with two in-compatible changes, case sensitive and hash32).
So here is the webrev for a strip-down version from the original
patch, in which it only
targets the nfd/nfc issue raised in 7130915 and 7168427. The
proposed changes for
case insensitive compare and hash32 for both java.io.File and
java.nio.file.Path are
removed.
http://cr.openjdk.java.net/~sherman/7130915_7168427/webrev
(The webrev for the "full" version has been moved to
http://cr.openjdk.java.net/~sherman/7130915_7168427/webrev_full)
Thanks,
-Sherman
I had to dig up the File Systems, Unicode, and Normalization
presentation [1] before reviewing this, it's been a while.
I think the changes for java.io for fine, I just worry that there may
be a few odd cases where it will be different to Apple JDK6. I looked
through the macosx-port/macosx-port/jdk forest and there is one patch
from Apple that does the NFC->NFD conversation. I don't get that patch
because you say that the syscalls handle NFC okay. In your changes
then you normalize when decoding the file names to Strings, which
seems right but that seems to be completely missing from the changes
that Apple brought over. The only minor comment is that we probably
need to check the return from core foundation functions such as
CFStringCreateMutable (this goes for the other native code too).
Adding the FileSystemProvider for MacOSX is great to see. The approach
is fine for but is somewhat inefficient when ->NFD or ->NFC is needed.
One inefficiency that can be fixed is in sun.nio.fs.MacOSXFileSystem.
normalizeJavaPath then you can eliminate the second call to
toCharArray. I think the new methods on sun.no.fs.UnixFileSystem
should be given comments so that it's clear whether they should be
overridden (the Unix* provider tends to be starting point for most
ports). A minor comment is that MacOSXNativeDispatcher has a bunch of
blank lines at the end, also I think "static final" is more normal
than "final static". At some point I think we should re-write
MacOSXNativeDispatcher.normalizepath so that it deosn't require the
critical section or malloc as in this area we try to keep the native
methods to a bare minimum.
Do the tests assume they are run on HFS? Just wondering if you you
need to look at the FileStore name/type to check. Some of variable
namesa bit non-standard, very C like. Minor nit is that the copyright
in the header isn't right in the tests. Also the tests list 2 CRs
whereas I assume this is one CR (with the other closed as a dup).
On case sensitive vs. insensitive then I think it should be
insensitive as per the first round but that would be a significant
change given that Apple's JDK does not appear to have changed
File.equals. Maybe we should think about this again once these changes
are in 8.
-Alan
[1]
http://developers.sun.com/global/products_platforms/solaris/reference/presentations/IUC29-FileSystems.pdf
