Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

It doesn’t add that much value.  I’ll take it out.

> On Jul 19, 2016, at 1:12 AM, Paul Sandoz  wrote:
> 
> 
>> On 18 Jul 2016, at 20:06, Steve Drach  wrote:
>> 
>>> 
>>> I would be inclined to drop the note for Enumeration and add a @see tag 
>>> referencing the Stream returned method.
>> 
>> I actually had to use the versionedEntries method for jdeps.  Because of 
>> that and because I don’t see the harm with leaving it in, I left it in.
>> 
> 
> You are implicitly making reference in the @apiNote note of entries() to a 
> method in the @apiNote of stream()
> 
> 533  * Iterator it = versionedStream(jf).iterator();
> 
> That’s quite confusing. If you wanna keep the @apiNote in entires i recommend 
> keep the two notes independent, and also add a @see to stream().
> 
> Paul.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Paul Sandoz]

> On 18 Jul 2016, at 20:06, Steve Drach  wrote:
> 
>> 
>> I would be inclined to drop the note for Enumeration and add a @see tag 
>> referencing the Stream returned method.
> 
> I actually had to use the versionedEntries method for jdeps.  Because of that 
> and because I don’t see the harm with leaving it in, I left it in.
> 

You are implicitly making reference in the @apiNote note of entries() to a 
method in the @apiNote of stream()

 533  * Iterator it = versionedStream(jf).iterator();

That’s quite confusing. If you wanna keep the @apiNote in entires i recommend 
keep the two notes independent, and also add a @see to stream().

Paul.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> It’s used in two places, moving it to jarPackages won’t work, I think.  I 
>> also put a check in to see if it’s a multi-release jar.
> I meant to move the method down to where jarPackages is located. I would 
> probably adjust the comment too but we can do that once your changes are in 
> jdk9/dev as we have several patches that will change this code. So I think 
> what you have is okay.

Okay.  Will do.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Alan Bateman]

On 18/07/2016 19:06, Steve Drach wrote:



It’s used in two places, moving it to jarPackages won’t work, I think. 
 I also put a check in to see if it’s a multi-release jar.
I meant to move the method down to where jarPackages is located. I would 
probably adjust the comment too but we can do that once your changes are 
in jdk9/dev as we have several patches that will change this code. So I 
think what you have is okay.


-Alan

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> Please review this change to JarFile that reverts JarFile::stream and 
>> JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
>> except line 504 in JarEntryIterator that now uses a different constructor to 
>> create a JarFileEntry.  I also added some implementation notes explaining 
>> how to create a versionedStream and a versionedEntries method.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8157524 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 
>> 

I put an updated webrev at 
http://cr.openjdk.java.net/~sdrach/8157524/webrev.01/index.html 



> For @implNote then you can put the example in {@code ...} and that will allow 
> to use angled brackets (no need for < etc).

Okay.  I doesn’t seem to allow nested @, so I just dropped the @Override 
annotations on the example

> 
> In ModulePath then it would be good to move versionedStream to jarPackages.

It’s used in two places, moving it to jarPackages won’t work, I think.  I also 
put a check in to see if it’s a multi-release jar.

> A comment on the method would be useful too although we have other changes 
> coming to this code so we can do it then.

I put a comment in

> 
> You should use an @apiNote, as this is presenting some interesting 
> information on how to use this method and not some particular characteristics 
> of the implementation.

Okay

> 
> I would be inclined to drop the note for Enumeration and add a @see tag 
> referencing the Stream returned method.

I actually had to use the versionedEntries method for jdeps.  Because of that 
and because I don’t see the harm with leaving it in, I left it in.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Paul Sandoz]

> On 15 Jul 2016, at 23:17, Steve Drach  wrote:
> 
> Hi,
> 
> Please review this change to JarFile that reverts JarFile::stream and 
> JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
> except line 504 in JarEntryIterator that now uses a different constructor to 
> create a JarFileEntry.  I also added some implementation notes explaining how 
> to create a versionedStream and a versionedEntries method.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8157524 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 
> 
> 

You should use an @apiNote, as this is presenting some interesting information 
on how to use this method and not some particular characteristics of the 
implementation.

I would be inclined to drop the note for Enumeration and add a @see tag 
referencing the Stream returned method.

Paul.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Alan Bateman]

On 15/07/2016 22:17, Steve Drach wrote:


Hi,

Please review this change to JarFile that reverts JarFile::stream and 
JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
except line 504 in JarEntryIterator that now uses a different constructor to 
create a JarFileEntry.  I also added some implementation notes explaining how 
to create a versionedStream and a versionedEntries method.

issue: https://bugs.openjdk.java.net/browse/JDK-8157524 

webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 



This looks okay to me.

For @implNote then you can put the example in {@code ...} and that will 
allow to use angled brackets (no need for < etc).


In ModulePath then it would be good to move versionedStream to 
jarPackages. A comment on the method would be useful too although we 
have other changes coming to this code so we can do it then.


-Alan

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Claes Redestad]

Hi,

this looks good to me, and it makes sense to me that upgrading to Java
9 and using this API on a Multi-release JAR doesn't suddenly filter out
entries.

/Claes

On 2016-07-15 23:17, Steve Drach wrote:

Hi,

Please review this change to JarFile that reverts JarFile::stream and 
JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
except line 504 in JarEntryIterator that now uses a different constructor to 
create a JarFileEntry.  I also added some implementation notes explaining how 
to create a versionedStream and a versionedEntries method.

issue: https://bugs.openjdk.java.net/browse/JDK-8157524 

webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 


Thanks,
Steve

RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

Hi,

Please review this change to JarFile that reverts JarFile::stream and 
JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
except line 504 in JarEntryIterator that now uses a different constructor to 
create a JarFileEntry.  I also added some implementation notes explaining how 
to create a versionedStream and a versionedEntries method.

issue: https://bugs.openjdk.java.net/browse/JDK-8157524 

webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 


Thanks,
Steve