Hi, Thanks for the review. I've filed a bug to track the cleanups suggested: https://bugs.openjdk.java.net/browse/JDK-8151860
Quick comments: 1) jrt fs is read-only file system as you noted. Copying content into jrt fs does not make sense. Eventually read-only file system exception is thrown. But, yes that cast can be avoided. 2) jrt-fs.jar is generated from jimage and jrt-fs sources and expected to work on previous JDK (jdk 8 in this case). This is to support cross-JDK reference to platform classes (from IDEs). So, it is better to avoid internal JDK classes such as sun.nio.fs.Globs in jrt-fs code. Thanks, -Sundar On 3/15/2016 4:51 AM, Xueming Shen wrote: > > (5) JrtFileSystemProvider.copy(src, dst, ...options) > > The "dst/target" should not be cast to "AbstractJrtPath"? the jrtfs > itself is > a readonly, I'm not sure if we want to support the operation of copying > a "file" output jrtfs. The copy/paste "copyToTarget" appears to be > functional, > if it's not a "AbstractJrtPath". > > > On 03/14/2016 04:08 PM, Xueming Shen wrote: >> (1) JrtFilePath: it does not seem to help performance to use the >> byte[] as the >> internal storage. >> >> (2) AbstractJrtFilesystem.java >> >> getPathMatcher: >> .... >> if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) { >> expr = JrtUtils.toRegexPattern(input); >> } else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) { >> expr = input; >> } else { >> throw new UnsupportedOperationException("Syntax '" + >> syntax >> + "' not recognized"); >> } >> >> (3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own copy? >> >> (4) JrtFilesystem.nodesToIterator() >> >> Stream has a "iterator() method. why need a collect() here? >> different performance? >> for example >> >> private Iterator<Path> nodesToIterator(AbstractJrtPath dir, >> List<Node> childNodes) { >> return childNodes.stream() >> .map(child -> >> (Path)dir.resolve(toJrtPath(child.getNameString()).getFileName())) >> .iterator(); >> } >> >> >> sherman >> >> >