On 3/15/2016 11:24 AM, Xueming Shen wrote:
>
> On 3/14/2016 7:48 PM, Sundararajan Athijegannathan wrote:
>> 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.
>
> Hi Sundar,
>
> What I meant is without that "check&cast-ing", the copyToTarget might
> actually work if the "dst/target" is a non-jrtfs-path", for example, copy
> a "class" file out of the jrtfs and store into the local file system
> (those code
> originally is designed/implemented to work that way, whether or not it
> works depends on if the src is readable and if the dst is writable). The
> question here is if this is something we want to do for the jrtfs.
> That is
> a design decision to make.
That's right. That has to be revisited.
-Sundar
>
> -Sherman
>
>
>
>>
>> 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
>>>>
>>>>
>
