Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 10/07/19 2:25 AM, Lance Andersen wrote:
> Hi Jaikiran,
>
> Again, thank you for your efforts here.
>
> Is there a CSR for this yet? 

There isn't one yet. I will need help on this part, since I haven't
created a CSR before. Is this something that I can create (I'm just a
"author" in the JDK project)? If yes, is there some specific things I
need to do?

I'll reply to the rest of the review comments soon. Thank you very much
for doing this review and offering to sponsor.

-Jaikiran


> This will need to approved prior to moving forward with committing the
> feature.
>
> I can sponsor once everything has been approved and finalized.
>
>
>
>
> ———
> @implSpec This method is a no-op if this compressor has already
> 886 * been previously closed,
>
> 
>
> Please remove “already” in both the close() and end() methods.  I
> believe the preference is the @implSpec and its relatives are on their
> own line as in https://openjdk.java.net/jeps/8068562 and was done for
> @apiNote earlier
>
>  - Undefined behavior after close()/end()
>
> I am not convinced the new wording is an improvement and I know Stuart
> was not thrilled with the existing wording.  However given the classes
> may be subclassed, I am not sure we  can truly specify the behavior
> which could be why the original authors used that wording.   Stuart
> thoughts?
>
>
> Outside of the @ImplSpec, I am not sure the initial wording for end()
> and close() really need to differ:
>
> end():
>
> Closes the decompressor and discards any unprocessed input.
>
> close():
>
> Releases resources held by this decompressor and discards any
> unprocessed input.This method should be called when the
> decompressor is no longer needed
>
>
>
>
>> On Jul 9, 2019, at 9:18 AM, Jaikiran Pai > > wrote:
>>
>> Can I please get a review and a sponsor for the patch that implements
>> the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8225763 ?
>>
>> There has been a recent discussion about this change in the
>> core-libs-dev mailing list here[1]. The latest version of the patch for
>> this change is now available at [2].
>>
>> Here's a summary of what's contained in this patch:
>>
>> - The Inflater/Deflater class now implements AutoCloseable
>>
>> - The class level javadoc of both these classes has been to updated to
>> use newer style of code, with try-with-resources, for the example
>> contained in that javadoc. The @apiNote in these class level javadoc has
>> also been updated to mention the use of close() method for releasing
>> resources.
>>
>> - The javadoc of end() method in both these classes has been updated to
>> encourage the use of close() method instead of this one. It now also has
>> a @implSpec which states that it's a no-op if called more than once.
>>
>> In addition, this javadoc has also been updated to replace the
>> "undefined behaviour" statement with a mention that the usage of the
>> Inflater/Deflater instance after a call to end() may throw an exception
>> in those subsequent usages. Please note that, there's no such explicit
>> mention in the javadoc of the (newly added) close() method because IMO,
>> it isn't needed for close() since I think it's kind of implied that a
>> closed resource can no longer be used for further operations.
>
> We need to be specific in close()  also for clarity
>>
>> - The new close() method has been added along with javadoc which uses an
>> @implSpec to state that it internally calls end()
>>
>> - TotalInOut.java test has been updated to use the new
>> try-with-resources construct for the inflater and deflater it uses.
>
> Please update @biug to include the bug number
>>
>> - A couple of new (testng) test classes have been added to test various
>> scenarios where close() and end() method get called. These test mostly
>> focus on ensuring that the close() and end(), either implicitly or
>> explicitly, get called the right number of times on the subclasses of
>> Inflater/Deflater.
>
> Overall they look OK.  In your tests, you are testing the number of
> calls for the sub-classes not for Deflate/Inflate so I would either
> update your comments to clarify that or pull them into their own test
> methods
>
>
> Again, thank you for your efforts here.
>
> Best
> Lance
> 
> 
> Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
>
>
>

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Stuart Marks]

1. New changeset with constant version of EnumSet.serialVersionUID.


This is already in the webrev.01 changeset. webrev.02 is an attempt to sneak 
the change without being visible in the serialized-form.html.


Ah. I skipped webrev.01 because I thought that webrev.02 had superseded it. 
Looking at webrev.01, I see



+// value computed from JDK 8 (and previous) EnumSet class
+// needed to properly cross-(de)serialize EnumSet.class objects
+// between JDK 8- <-> JDK 9+
+private static final long serialVersionUID = 1009687484059888093L;
+


I don't think this comment can cover the entire history here. We'll have to rely 
on the bug report, the CSR, and the email archives.


Most declarations of serialVersionUID don't have a comment at all. So, we could 
just omit it.


If you feel a comment is necessary, perhaps something like

// declare serialization compatibility with JDK 8 (see JDK-8227368)

might be sufficient. Otherwise, webrev.01 looks fine.


On 7/9/19 1:57 AM, Stuart Marks wrote:
2. Create draft CSR. 


Created:

https://bugs.openjdk.java.net/browse/JDK-8227432


I've done some editing on this CSR and I've marked myself as a reviewer. Please 
move this to Finalized.


While we're waiting for the CSR to be approved (I hope this takes only a day or 
two) I'll do some testing with your patch.


Thanks,

s'marks

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.


Thanks,

Brian

> On Jul 9, 2019, at 2:26 PM, Jason Mehrens  wrote:
> 
> Would the SecurityManager need to for permissions (checkWrite or some new 
> permission) before cancelDeleteOnExit() is allowed?

JDK 14 RFR of JDK-8202385: Annotation to mark serial-related fields and methods

[Joe Darcy]

Hello,

Returning to some old work [1], please review the addition of a 
java.io.Serial annotation type for JDK 14 to mark serial-related fields 
and methods:


    webrev: http://cr.openjdk.java.net/~darcy/8202385.3/
    CSR: https://bugs.openjdk.java.net/browse/JDK-8217698

Thanks,

-Joe

[1] Previous review threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/053055.html

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054801.html

Re: RFR [14/java.xml] 7148925: StAXSource causes exceptions to be thrown with certain wellformed XML instances

[Joe Wang]

On 7/9/19 3:07 PM, Lance Andersen wrote:

Hi Joe,

Overall, this looks good.


Thanks Lance!


Please add the bug number to the top @bug in the test class.


Added.



I might have defined all of the elements in the DataProvider “xml” in 
separate String objects to make it easier to read, but no big deal and 
does not need changed prior to pushing.


Thanks. I'm going to keep them this way then ;-)  They are small enough.

-Joe





On Jul 9, 2019, at 5:17 PM, Joe Wang > wrote:


Please review a fix for an Exception caused by StAXSource. The fix 
adds code that processes the prolog and misc content as specified by 
the XML specification, and removes the code that made incorrect 
assumption about XML document structure.


https://bugs.openjdk.java.net/browse/JDK-7148925
http://cr.openjdk.java.net/~joehw/jdk14/7148925/webrev/

Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR [14/java.xml] 7148925: StAXSource causes exceptions to be thrown with certain wellformed XML instances

[Lance Andersen]

Hi Joe,

Overall, this looks good.

Please add the bug number to the top @bug in the test class.


I might have defined all of the elements in the DataProvider “xml” in separate 
String objects to make it easier to read, but no big deal and does not need 
changed prior to pushing.




> On Jul 9, 2019, at 5:17 PM, Joe Wang  wrote:
> 
> Please review a fix for an Exception caused by StAXSource. The fix adds code 
> that processes the prolog and misc content as specified by the XML 
> specification, and removes the code that made incorrect assumption about XML 
> document structure.
> 
> https://bugs.openjdk.java.net/browse/JDK-7148925
> http://cr.openjdk.java.net/~joehw/jdk14/7148925/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would the SecurityManager need to for permissions (checkWrite or some new 
permission) before cancelDeleteOnExit() is allowed?

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 1:08 PM
To: core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


> On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
> wrote:
>
>> Since deleteOnExit() is an deliberate act, perhaps there should be a 
>> corresponding withdrawDeleteOnExit() that reverses it so that it is 
>> intentional and not a side-effect of other File methods.
>
> I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/

Of course a CSR would be called for if this is agreed upon.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Brian,

Just a note, one issue I see with webrev.01 is that JDK-7092892 is not 
resolved.  On one hand more calls to DeleteOnExitHook should trigger class init 
sooner avoiding the issue.  On the other it seems this could be more methods 
that could fail by throwing ExceptionInInitializerError that wouldn't have 
before the change.  I would think that you would want to mark JDK-7092892 as 
blocker of JDK-8193072 before you go this route.

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 10:07 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:

http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
>
> The sequence described is the specified behavior of the API, whether it is a 
> developer mistake or not is unknowable but it would be a compatibility issue 
> to change it. The filename is the key and there is no way to determine if it 
> is the original file or a replacement. deleteOnExit Wins!

RFR [14/java.xml] 7148925: StAXSource causes exceptions to be thrown with certain wellformed XML instances

[Joe Wang]

Please review a fix for an Exception caused by StAXSource. The fix adds 
code that processes the prolog and misc content as specified by the XML 
specification, and removes the code that made incorrect assumption about 
XML document structure.


https://bugs.openjdk.java.net/browse/JDK-7148925
http://cr.openjdk.java.net/~joehw/jdk14/7148925/webrev/

Thanks,
Joe

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Lance Andersen]

Hi Jaikiran,

Again, thank you for your efforts here.

Is there a CSR for this yet?  This will need to approved prior to moving 
forward with committing the feature.

I can sponsor once everything has been approved and finalized.




———
@implSpec This method is a no-op if this compressor has already
 886  * been previously closed,



Please remove “already” in both the close() and end() methods.  I believe the 
preference is the @implSpec and its relatives are on their own line as in 
https://openjdk.java.net/jeps/8068562  
and was done for @apiNote earlier

 - Undefined behavior after close()/end()

I am not convinced the new wording is an improvement and I know Stuart was not 
thrilled with the existing wording.  However given the classes may be 
subclassed, I am not sure we  can truly specify the behavior which could be why 
the original authors used that wording.   Stuart thoughts?


Outside of the @ImplSpec, I am not sure the initial wording for end() and 
close() really need to differ:

end():

Closes the decompressor and discards any unprocessed input.

close():

Releases resources held by this decompressor and discards any unprocessed 
input.This method should be called when the decompressor is no longer needed



> On Jul 9, 2019, at 9:18 AM, Jaikiran Pai  wrote:

> 
> Can I please get a review and a sponsor for the patch that implements
> the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8225763 ?
> 
> There has been a recent discussion about this change in the
> core-libs-dev mailing list here[1]. The latest version of the patch for
> this change is now available at [2].
> 
> Here's a summary of what's contained in this patch:
> 
> - The Inflater/Deflater class now implements AutoCloseable
> 
> - The class level javadoc of both these classes has been to updated to
> use newer style of code, with try-with-resources, for the example
> contained in that javadoc. The @apiNote in these class level javadoc has
> also been updated to mention the use of close() method for releasing
> resources.
> 
> - The javadoc of end() method in both these classes has been updated to
> encourage the use of close() method instead of this one. It now also has
> a @implSpec which states that it's a no-op if called more than once.
> 
> In addition, this javadoc has also been updated to replace the
> "undefined behaviour" statement with a mention that the usage of the
> Inflater/Deflater instance after a call to end() may throw an exception
> in those subsequent usages. Please note that, there's no such explicit
> mention in the javadoc of the (newly added) close() method because IMO,
> it isn't needed for close() since I think it's kind of implied that a
> closed resource can no longer be used for further operations.

We need to be specific in close()  also for clarity
> 
> - The new close() method has been added along with javadoc which uses an
> @implSpec to state that it internally calls end()
> 
> - TotalInOut.java test has been updated to use the new
> try-with-resources construct for the inflater and deflater it uses.

Please update @biug to include the bug number
> 
> - A couple of new (testng) test classes have been added to test various
> scenarios where close() and end() method get called. These test mostly
> focus on ensuring that the close() and end(), either implicitly or
> explicitly, get called the right number of times on the subclasses of
> Inflater/Deflater.

Overall they look OK.  In your tests, you are testing the number of calls for 
the sub-classes not for Deflate/Inflate so I would either update your comments 
to clarify that or pull them into their own test methods


Again, thank you for your efforts here.

Best
Lance
 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: The final optimized version of Dual-Pivot Quicksort (ver.19.1)

[Brent Christian]

Hi,

Is the webrev incomplete?  It only contains the changes for 
DualPivotQuicksort.java, and not for Arrays.java, etc.


Thanks,
-Brent

On 6/18/19 2:37 PM, Vladimir Yaroslavskiy wrote:

Hi team,

Please review the final optimized version of Dual-Pivot Quicksort (ver.19.1),
see http://cr.openjdk.java.net/~alanb/8226297/0/webrev
(link to Jira task https://bugs.openjdk.java.net/browse/JDK-8226297)

The new version was published here more than one year ago (on Jan 19, 2018).
Since that time Dual-Pivot Quicksort has been significantly improved
and this work was done in collaboration with Doug Lea and Laurent Bourges.

You can find summary of all changes below.

DualPivotQuicksort.java
--
* The 1-st and 5-th candidates are taken as pivots
   instead of 2-nd and 4-th
* Pivots are chosen with another step
* Pivot candidates are sorted by combination of
   5-element network sorting and insertion sort
* New backwards partitioning was introduced
* Partitioning is related to the golden ratio
* Quicksort tuning parameters were updated
* Thresholds for byte / char / short sorting were updated
* Additional steps for float / double were fully rewritten
* Parallel sorting is based on combination of merge sort
   and Dual-Pivot Quicksort
* Pair insertion sort was optimized and became a part
   of mixed insertion sort
* Mixed insertion sort was introduced: combination
   of simple insertion sort, pin insertion sort and
   pair insertion sort
* Simple insertion sort is invoked on tiny array
* Pin insertion sort is started on small array
* Pair insertion sort is continued on remain part
* Merging of runs is invoked on each iteration of Quicksort
* Merging of runs was fully rewritten
* Optimal partitioning of merging is used
* Not more than one copy of part to buffer during merging
* Merging parameters were updated
* Initial capacity of runs is based on size
* Max number of runs is constant
* Optimized version of heap sort was introduced
* Heap sort is used as a guard against quadratic time
   (int / long / float / double)
* Parallel merging of runs was also provided
* Parallel sorting, heap sort and merging of runs
   are not used in byte / char / short sorting
* Counting sort for byte / char / short were optimized
* Counting sort is used as a guard against quadratic time
   (byte / char / short)
* Code style and javadoc improvements

Sorting.java
--
* New test cases were added
* Test cases are invoked for both: sequential and
   parallel sorting
* Check for Object sorting was added
* New tests were added against heap sort
* Added test case against bug in parallel merge
   sort on float / double data

ParallelSorting.java
--
* This class was removed, because Sorting class
   covers all cases

SortingHelper.java
--
* The helper class provides access package-private
   methods of DualPivotQuicksort class during testing

Arrays.java
--
* Calls of Dual-Pivot Quicksort were updated
* Parallel sorting of primitives was switched to parallel
   Dual-Pivot Quicksort
* Javadoc was updated, double spaces were removed
* Format changes

ArraysParallelSortHelpers.java
--
* Implementation of parallel sorting
   for primitives was removed
* Javadoc was updated

Thank you,
Vladimir

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
> wrote:
> 
>> Since deleteOnExit() is an deliberate act, perhaps there should be a 
>> corresponding withdrawDeleteOnExit() that reverses it so that it is 
>> intentional and not a side-effect of other File methods.
> 
> I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/

Of course a CSR would be called for if this is agreed upon.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Hi Roger,

> On Jul 9, 2019, at 8:25 AM, Roger Riggs  wrote:
> 
> The interesting part will be writing/updating the specification to make it 
> clear what happens and under what conditions.
> How often are File instances re-used vs creating new ones.
> And any interactions with other APIs that create or delete files with the 
> same name.  (file channels, zip, etc…)

These interactions also occurred to me. They would be impossible to account for.

> Since deleteOnExit() is an deliberate act, perhaps there should be a 
> corresponding withdrawDeleteOnExit() that reverses it so that it is 
> intentional and not a side-effect of other File methods.

I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Roger Riggs]

Hi Brian,

The interesting part will be writing/updating the specification to make 
it clear what happens and under what conditions.

How often are File instances re-used vs creating new ones.
And any interactions with other APIs that create or delete files with 
the same name.  (file channels, zip, etc...)


Since deleteOnExit() is an deliberate act, perhaps there should be a 
corresponding withdrawDeleteOnExit() that reverses it so that it is 
intentional and not a side-effect of other File methods.


Roger


On 7/9/19 11:07 AM, Brian Burkhalter wrote:

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:


http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

On Jul 9, 2019, at 7:34 AM, Roger Riggs > wrote:


The sequence described is the specified behavior of the API, whether 
it is a developer mistake or not is unknowable but it would be a 
compatibility issue to change it. The filename is the key and there 
is no way to determine if it is the original file or a replacement. 
deleteOnExit Wins!

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[David Lloyd]

On Tue, Jul 9, 2019 at 8:19 AM Jaikiran Pai  wrote:
> - In addition, I have sneaked in an unrelated change in this patch, into
> the Deflater.end() method:
>
>  public void end() {
>  synchronized (zsRef) {
> +// check if already closed
> +if (zsRef.address() == 0) {
> +return;
> +}
>  zsRef.clean();
>  input = ZipUtils.defaultBuf;
> +inputArray = null;
> +}
>
> Unlike in the Inflater.end(), the Deflater.end() didn't previously have
> the "inputArray = null". I have included it in this patch, since it
> looks right to clean up that array too. If this isn't the right
> time/patch to do it, please do let me know and I'll take that up separately.

This was probably my mistake.  The fix looks good to me!
-- 
- DML

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:

http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
> 
> The sequence described is the specified behavior of the API, whether it is a 
> developer mistake or not is unknowable but it would be a compatibility issue 
> to change it. The filename is the key and there is no way to determine if it 
> is the original file or a replacement. deleteOnExit Wins!

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Roger Riggs]

Hi Brian,

The sequence described is the specified behavior of the API, whether it 
is a developer mistake or not is unknowable but it would be a 
compatibility issue to change it. The filename is the key and there is 
no way to determine if it is the original file or a replacement. 
deleteOnExit Wins!


$.02, Roger


On 7/8/19 5:08 PM, Brian Burkhalter wrote:

Ivan / Jason,

Thanks for the good observations.


On Jul 8, 2019, at 1:35 PM, Ivan Gerasimov  wrote:

I believe this would introduce a behavior change in a scenario lile:
File f = ...;
f.deleteOnExit();
f.delete();
f.createNewFile();

I.e. when the with the same name is recreated.  Current behavior is to unlink 
such a file before exiting, no matter if it were explicitly deleted and then 
recreated or not.

Good point. Given this consideration I am not sure that this bug can be fixed.


On Jul 8, 2019, at 1:53 PM, Jason Mehrens  wrote:

Previously File.delete wouldn't throw IllegalStateException and with this patch 
it looks like that is possible (and not desirable).  I would think that this 
change could the break java.util.logging.FileHandler because Handler.close runs 
in a shutdown hook.


I think you are correct that the ISE should not be thrown.

Thanks,

Brian

Re: [13] RFR: 8227127: Era designator not displayed correctly using the COMPAT provider

[Roger Riggs]

Hi Naoto,

Looks fine.

Thanks, Roger

On 7/8/19 6:11 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8227127

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8227127/webrev.00/

This is a regression caused by the fix to 8039301, where era display 
names are correctly retrieved with styles. This revealed that the 
short names for Gregorian eras are missing in COMPAT locale provider. 
Providing the proper display names in its resource bundles fixes the 
issue.


Naoto

Re: RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string

[Roger Riggs]

Hi Thejasvi,

Looks good, thanks for the updates

Roger

On 7/9/19 3:50 AM, Thejasvi Voniadka wrote:

Hi Roger,

Please find the updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.4/


I have updated both lines 3924 and 3874 of 
.../java/time/format/DateTimeFormatterBuilder.java.

I have reduced the number of tests to just 3, and the number of locales to just 
1 in java/time/format/TestLocalizedOffsetPrinterParser.java. That should 
provide sufficient coverage towards the functionality.



-Original Message-
From: Thejasvi Voniadka
Sent: Friday, July 05, 2019 9:16 AM
To: Roger Riggs ; core-libs-dev@openjdk.java.net; 
i18n-...@openjdk.java.net
Subject: RE:  RFR: 8154520: java.time: appendLocalizedOffset() should return 
the localized "GMT" string

Hi Roger,

  


Thank you for the review. I am in transit this weekend, but I will fix this and 
republish as soon as I can.

  

  


From: Roger Riggs
Sent: Wednesday, July 03, 2019 10:46 PM
To: Thejasvi Voniadka ; 
core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should return 
the localized "GMT" string

  


Hi,

Looks ok, but...

.../java/time/format/DateTimeFormatterBuilder.java
3924: needs a space in "if(" -> "if ("

java/time/format/TestLocalizedOffsetPrinterParser.java

I would cut the number of test cases to a minimum; only need to ensure the code 
is correct.
We don't need to be testing the CLDR data; it is just  pass through.
At least cut the number of different locale's used to cut the risk of 
unexpected maintenance.

Thanks, Roger




On 7/3/19 12:10 PM, HYPERLINK 
"mailto:naoto.s...@oracle.com"naoto.s...@oracle.com wrote:

Looks good.

Naoto

On 7/3/19 5:32 AM, Thejasvi Voniadka wrote:



Hi Naoto,

Thank you for the review. Please find the updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.3/


The TCKOffsetPrinterParser.java has been reverted back to what it was, except 
for the copyright year and the locale addition. I have incorporated your 
comments to TestLocalizedOffsetPrinterParser.java.



-Original Message-
From: Naoto Sato
Sent: Tuesday, July 02, 2019 9:33 PM
To: Thejasvi Voniadka HYPERLINK 
"mailto:thejasvi.v.vonia...@oracle.com;; HYPERLINK 
"mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; HYPERLINK 
"mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should return 
the localized "GMT" string

Hi Thejasvi,

Here are my comments to the webrev:

TCKOffsetPrinterParser.java

- No need to define Locale_US as a static field, nor have it in the test data 
(data_print_localized) then pass it as an argument to the test.
Specifying Locale.US in line 572, 578, and 586 should suffice.

TestOffsetPrinterParser.java

- Copyright year is 2019.

- It would be nice if some comments that reads something like "This test relies on the 
localized text of "GMT" in the CLDR."

- Test class (and the description) should include "Localized", as it is testing 
the implementation of localized version of OffsetIdPrinterParser.

- Line 64-76: I prefer just instantiating them in the test data, not defining a 
static field for each, unless there will be duplicate in the test data.

- Line 111, 118, 124, 132: I believe the locale parameter is required.
Make sure that with Objects.requireNonNull(), or fail if it's null.

Naoto

On 7/2/19 7:32 AM, Thejasvi Voniadka wrote:



Hi Naoto,

Thank you for the review. I have performed the modifications, and here is the 
updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.2/


I have moved the new tests from TCK area. I have also updated the current TCK 
test to explicitly pass Locale.US while calling format.




-Original Message-
From: Naoto Sato
Sent: Monday, July 01, 2019 9:02 PM
To: Thejasvi Voniadka HYPERLINK 
"mailto:thejasvi.v.vonia...@oracle.com;;
HYPERLINK "mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; 
HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time:
appendLocalizedOffset() should return the localized "GMT" string

Hi Thejasvi,

Thanks for fixing this.

Since those new test cases depend on the CLDR localization, which might change in other 
implementations, those test cases should be in jdk/java/time/test directory, as 
"tck" tests should only test the spec.
Please create a new test case for this in the "test" directory (with @modules 
jdk.localedata directive) similar to the existing TCK one. Also the current test in the TCK should 
enforce that it runs in Locale.US so that the result should match "GMT."

Naoto

On 6/28/19 5:59 AM, Thejasvi Voniadka wrote:



Hi,

Request you to please review this change.


JBS:    https://bugs.openjdk.java.net/browse/JDK-8154520


Description:    At present, the "DateTimeFormatterBuilder.appendLocalizedOffset()" method formulates the base string as "GMT", 

Re: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13

[Roger Riggs]

+1

On 7/9/19 9:07 AM, naoto.s...@oracle.com wrote:

Looks good, Ramanand.

Naoto

On 7/9/19 5:09 AM, Ramanand Patil wrote:

Hi Naoto,
Thank you for the review. Revised webrev: 
http://cr.openjdk.java.net/~rpatil/8224560/webrev.02/



I plan to push the changes tomorrow, if there are no further comments.


Regards,
Ramanand.


-Original Message-
From: Naoto Sato
Sent: Monday, July 8, 2019 11:19 PM
To: Ramanand Patil ; Andrew John Hughes
; core-libs-dev@openjdk.java.net; i18n-
d...@openjdk.java.net
Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
tzdata2019a and 8225580: tzdata2018i integration causes test 
failures on jdk-

13

Hi Ramanand,

As to the change in ZoneInfoFile.java, I would put that special 
handling of
Gaza/Hebron in line 577, as well as merging the comment in 575,576, 
so that

it won't duplicate the code.

Otherwise looks good.

Naoto

On 7/8/19 3:35 AM, Ramanand Patil wrote:

Hi Andrew,
Thank you for your review.
Updated webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.01/

Regards,
Ramanand.


-Original Message-
From: Andrew John Hughes 
Sent: Saturday, July 6, 2019 9:53 PM
To: Ramanand Patil ; core-libs-
d...@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
tzdata2019a and 8225580: tzdata2018i integration causes test failures
on jdk-
13



On 05/07/2019 15:16, Ramanand Patil wrote:

Hi all,
Please review the patch for tzdata2019a integration into jdk 
project.

Webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.00/
Bugs: https://bugs.openjdk.java.net/browse/JDK-8224560
https://bugs.openjdk.java.net/browse/JDK-8225580

Summary:
- The fix contains cumulative tzdata changes from tzdata2018i and

tzdata2019a, as tzdata2018i was not integrated into jdk/jdk earlier.

- In JDK-13/14, multiple failures were seen during integration of
tzdata2018i

(JDK-8225580), those are fixed now. Many thanks to Naoto for
providing a fix for this in CLDRConverter.java.

I would guess this is due to the CLDR update (JDK-8221432: Upgrade
CLDR to Version 35.1) in OpenJDK 13, making TimeZone.getAvailableIDs
inappropriate in some way?

Fix looks good. One minor change:

+    AVAILABLE_TZIDS = new
+ HashSet(ZoneId.getAvailableZoneIds());

is missing the  or <>:

+    AVAILABLE_TZIDS = new
+ HashSet<>(ZoneId.getAvailableZoneIds());

Will this fix also resolve JDK-8225580? If so, it's probably worth
mentioning both bug IDs in the commit.
Yes, this fix will also resolve JDK-8225580, hence included in the 
subject

line.

And thank you, I will add both bug IDs in the commit message.



- There are 2 type of test failures in TestZoneInfo310.java file,
which are

solved in this patch by providing workarounds, But a permanent fix
needs to be added in future for the same. Below are the 2 bugs
created to track the development on it:

1. https://bugs.openjdk.java.net/browse/JDK-8223388:

TestZoneInfo310.java fails post tzdata2018i integration:

This failure is seen for the TimeZones which are having zone rules
defined

till year 2037 or beyond. For now, the failing zones are skipped.

The supporting test class ZoneInfo.java has maxYear defined

http://hg.openjdk.java.net/jdk/jdk/file/d01b345865d7/test/jdk/sun/uti
l/cale ndar/zi/Zoneinfo.java#l40, changing this max value greater
than the zone rule's last year also fixes the issue, but further
investigation is needed on why this boundary condition is affecting
the test behavior.

I wonder if 2037 is in someway related to the rollover of 32-bit time

values?
I think, not directly related but how the test and JDK handles 
these values.
In JDK, the transitions beyond 2037 are delegated to 
SimpleTimeZone, and I

think the test somehow miscalculates it.

http://hg.openjdk.java.net/jdk/jdk/file/5919b273def6/src/java.base/sha
re/classes/sun/util/calendar/ZoneInfo.java#l48
I think, I should re-visit and see if these test are really needed
now. As per the long standing bug JDK-8166983 suggestion was to remove
the whole tests from test/sun/util/calendar/zi



2. JDK-8227293: https://bugs.openjdk.java.net/browse/JDK-8227293

TestZoneInfo310.java fails post tzdata2019a integration for Palestine
zone
rules:

There are many hacks and assumptions in the class
sun.util.calendar.ZoneInfoFile.java. This issue looks because of the
code starting from here:
http://hg.openjdk.java.net/jdk/jdk/file/963924f1c891/src/java.base/s
ha
re/classes/sun/util/calendar/ZoneInfoFile.java#l552
There is an assumption where the transition date is >=24,(line 577
and 599)

it assumes it is the "last" rule, but it is not last rule in case of
Asia/Gaza and Asia/Hebron zones.

For now, I have fixed these 2 problematic timezones by overwriting
the assumption made on line 577, where date of month dom =
startRule.dom; I

think, overriding of the second jdk hack on line 599 is not required
as the "dom" is calculated from the last rule there. Keeping this bug
open as we need to find a generic solution 

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 08/07/19 11:16 PM, Lance Andersen wrote:
> Hi Jaikiran,
>
> Thank you for your efforts here.
>
>> On Jul 6, 2019, at 9:59 AM, Jaikiran Pai > > wrote:
>>
>>>
>>>  - Idempotency
>>>
>>> Yes, it looks to me like Inflater.end() and Deflater.end()
>>> implementations are idempotent. As you point out, overrides by
>>> subclasses might not be. We should be clear when we're talking about
>>> the specific implementatations of the end() methods in the Deflater
>>> and Inflater classes, or whether we're talking about the contracts
>>> defined by these method specifications on these classes and subtypes.
>>>
>>> The behavior of an implementation in a class can be specified with
>>> @implSpec without imposing this on the contract of subclasses. This is
>>> useful for subclasses that need to know the exact behavior of calling
>>> super.end(), and also for callers who know they have an instance of a
>>> particular class and not a subclass.
>>>
>>> The upshot is that while we might not have the end() method's contract
>>> specify idempotency, we can certainly say so in an @implSpec, if doing
>>> this will help. I'm not sure we'll actually need it in this case, but
>>> let's keep it in mind.
>>
>> Thank you. Although not for end(), I have now used this @implSpec in the
>> first version of my patch[1].
>
> This should be done for end() as well to set expectations.  While
> close() is the preferred way to go moving forward, end() is not going
> anywhere and still needs to be a first class-citizen WRT documentation.

Done. I have added the @implSpec for end() too in the new updated
revision of the webrev. I have opened a separate RFR thread containing
that webrev.


>>>
>>> **
>>>
>>> If you think the issues are settled enough, maybe it's time for you to
>>> take a stab at a patch. 
>>
>> The initial version of my patch is now ready at [1]. Here's a high level
>> summary of what's contained in this patch:
>
> Please start a review request thread so it is easier to follow.  Once
> you do that, I will reply to it..

Done - I have now created a new RFR thread for this here
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061229.html


>
> Also, reminder to update copyright dates.

The updated webrev in the RFR thread contains this update.


>
> For the tests, it would be best to add more comments.  Out of
> curiosity, was there a reason you chose not to use TestNG vs having
> just a main which invokes each test?

No specific reason. I'm still relatively new to the JDK codebase and
don't know when to use testng as against the regular main() driven
tests. I have now cleaned up the tests and converted them to testng in
the new webrev that I proposed.

-Jaikiran

RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Can I please get a review and a sponsor for the patch that implements
the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8225763 ?

There has been a recent discussion about this change in the
core-libs-dev mailing list here[1]. The latest version of the patch for
this change is now available at [2].

Here's a summary of what's contained in this patch:

- The Inflater/Deflater class now implements AutoCloseable

- The class level javadoc of both these classes has been to updated to
use newer style of code, with try-with-resources, for the example
contained in that javadoc. The @apiNote in these class level javadoc has
also been updated to mention the use of close() method for releasing
resources.

- The javadoc of end() method in both these classes has been updated to
encourage the use of close() method instead of this one. It now also has
a @implSpec which states that it's a no-op if called more than once.

In addition, this javadoc has also been updated to replace the
"undefined behaviour" statement with a mention that the usage of the
Inflater/Deflater instance after a call to end() may throw an exception
in those subsequent usages. Please note that, there's no such explicit
mention in the javadoc of the (newly added) close() method because IMO,
it isn't needed for close() since I think it's kind of implied that a
closed resource can no longer be used for further operations.

- The new close() method has been added along with javadoc which uses an
@implSpec to state that it internally calls end()

- TotalInOut.java test has been updated to use the new
try-with-resources construct for the inflater and deflater it uses.

- A couple of new (testng) test classes have been added to test various
scenarios where close() and end() method get called. These test mostly
focus on ensuring that the close() and end(), either implicitly or
explicitly, get called the right number of times on the subclasses of
Inflater/Deflater.

- I have run the tests under test/jdk/java/util/zip/ by running:

jtreg -jdk:build/macosx-x86_64-server-release/images/jdk
test/jdk/java/util/zip/

and they have come back passing (except for failures/errors in
java/util/zip/ZipFile/TestZipFile.java, java/util/zip/3GBZipFiles.sh and
java/util/zip/DataDescriptorSignatureMissing.java - those issues though
are unrelated to this change and expected to fail, based on what I see
in their test report logs)

- In addition, I have sneaked in an unrelated change in this patch, into
the Deflater.end() method:

 public void end() {
 synchronized (zsRef) {
+    // check if already closed
+    if (zsRef.address() == 0) {
+    return;
+    }
 zsRef.clean();
 input = ZipUtils.defaultBuf;
+    inputArray = null;
+    }

Unlike in the Inflater.end(), the Deflater.end() didn't previously have
the "inputArray = null". I have included it in this patch, since it
looks right to clean up that array too. If this isn't the right
time/patch to do it, please do let me know and I'll take that up separately.


[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-June/061096.html

[2] http://cr.openjdk.java.net/~jpai/webrev/8225763/2/webrev/

-Jaikiran

Re: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13

[naoto . sato]

Looks good, Ramanand.

Naoto

On 7/9/19 5:09 AM, Ramanand Patil wrote:

Hi Naoto,
Thank you for the review. Revised webrev: 
http://cr.openjdk.java.net/~rpatil/8224560/webrev.02/


I plan to push the changes tomorrow, if there are no further comments.


Regards,
Ramanand.


-Original Message-
From: Naoto Sato
Sent: Monday, July 8, 2019 11:19 PM
To: Ramanand Patil ; Andrew John Hughes
; core-libs-dev@openjdk.java.net; i18n-
d...@openjdk.java.net
Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-
13

Hi Ramanand,

As to the change in ZoneInfoFile.java, I would put that special handling of
Gaza/Hebron in line 577, as well as merging the comment in 575,576, so that
it won't duplicate the code.

Otherwise looks good.

Naoto

On 7/8/19 3:35 AM, Ramanand Patil wrote:

Hi Andrew,
Thank you for your review.
Updated webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.01/

Regards,
Ramanand.


-Original Message-
From: Andrew John Hughes 
Sent: Saturday, July 6, 2019 9:53 PM
To: Ramanand Patil ; core-libs-
d...@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
tzdata2019a and 8225580: tzdata2018i integration causes test failures
on jdk-
13



On 05/07/2019 15:16, Ramanand Patil wrote:

Hi all,
Please review the patch for tzdata2019a integration into jdk project.
Webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.00/
Bugs: https://bugs.openjdk.java.net/browse/JDK-8224560
https://bugs.openjdk.java.net/browse/JDK-8225580

Summary:
- The fix contains cumulative tzdata changes from tzdata2018i and

tzdata2019a, as tzdata2018i was not integrated into jdk/jdk earlier.

- In JDK-13/14, multiple failures were seen during integration of
tzdata2018i

(JDK-8225580), those are fixed now. Many thanks to Naoto for
providing a fix for this in CLDRConverter.java.

I would guess this is due to the CLDR update (JDK-8221432: Upgrade
CLDR to Version 35.1) in OpenJDK 13, making TimeZone.getAvailableIDs
inappropriate in some way?

Fix looks good. One minor change:

+AVAILABLE_TZIDS = new
+ HashSet(ZoneId.getAvailableZoneIds());

is missing the  or <>:

+AVAILABLE_TZIDS = new
+ HashSet<>(ZoneId.getAvailableZoneIds());

Will this fix also resolve JDK-8225580? If so, it's probably worth
mentioning both bug IDs in the commit.

Yes, this fix will also resolve JDK-8225580, hence included in the subject

line.

And thank you, I will add both bug IDs in the commit message.



- There are 2 type of test failures in TestZoneInfo310.java file,
which are

solved in this patch by providing workarounds, But a permanent fix
needs to be added in future for the same. Below are the 2 bugs
created to track the development on it:

1. https://bugs.openjdk.java.net/browse/JDK-8223388:

TestZoneInfo310.java fails post tzdata2018i integration:

This failure is seen for the TimeZones which are having zone rules
defined

till year 2037 or beyond. For now, the failing zones are skipped.

The supporting test class ZoneInfo.java has maxYear defined

http://hg.openjdk.java.net/jdk/jdk/file/d01b345865d7/test/jdk/sun/uti
l/cale ndar/zi/Zoneinfo.java#l40, changing this max value greater
than the zone rule's last year also fixes the issue, but further
investigation is needed on why this boundary condition is affecting
the test behavior.

I wonder if 2037 is in someway related to the rollover of 32-bit time

values?

I think, not directly related but how the test and JDK handles these values.
In JDK, the transitions beyond 2037 are delegated to SimpleTimeZone, and I

think the test somehow miscalculates it.

http://hg.openjdk.java.net/jdk/jdk/file/5919b273def6/src/java.base/sha
re/classes/sun/util/calendar/ZoneInfo.java#l48
I think, I should re-visit and see if these test are really needed
now. As per the long standing bug JDK-8166983 suggestion was to remove
the whole tests from test/sun/util/calendar/zi



2. JDK-8227293: https://bugs.openjdk.java.net/browse/JDK-8227293

TestZoneInfo310.java fails post tzdata2019a integration for Palestine
zone
rules:

There are many hacks and assumptions in the class
sun.util.calendar.ZoneInfoFile.java. This issue looks because of the
code starting from here:
http://hg.openjdk.java.net/jdk/jdk/file/963924f1c891/src/java.base/s
ha
re/classes/sun/util/calendar/ZoneInfoFile.java#l552
There is an assumption where the transition date is >=24,(line 577
and 599)

it assumes it is the "last" rule, but it is not last rule in case of
Asia/Gaza and Asia/Hebron zones.

For now, I have fixed these 2 problematic timezones by overwriting
the assumption made on line 577, where date of month dom =
startRule.dom; I

think, overriding of the second jdk hack on line 599 is not required
as the "dom" is calculated from the last rule there. Keeping this bug
open as we need to find a generic solution for this issue, without
hard-coding the values and 

RE: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13

[Ramanand Patil]

Hi Naoto,
Thank you for the review. Revised webrev: 
http://cr.openjdk.java.net/~rpatil/8224560/webrev.02/


I plan to push the changes tomorrow, if there are no further comments.


Regards,
Ramanand.

> -Original Message-
> From: Naoto Sato
> Sent: Monday, July 8, 2019 11:19 PM
> To: Ramanand Patil ; Andrew John Hughes
> ; core-libs-dev@openjdk.java.net; i18n-
> d...@openjdk.java.net
> Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
> tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-
> 13
> 
> Hi Ramanand,
> 
> As to the change in ZoneInfoFile.java, I would put that special handling of
> Gaza/Hebron in line 577, as well as merging the comment in 575,576, so that
> it won't duplicate the code.
> 
> Otherwise looks good.
> 
> Naoto
> 
> On 7/8/19 3:35 AM, Ramanand Patil wrote:
> > Hi Andrew,
> > Thank you for your review.
> > Updated webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.01/
> >
> > Regards,
> > Ramanand.
> >
> >> -Original Message-
> >> From: Andrew John Hughes 
> >> Sent: Saturday, July 6, 2019 9:53 PM
> >> To: Ramanand Patil ; core-libs-
> >> d...@openjdk.java.net; i18n-...@openjdk.java.net
> >> Subject: Re:  RFR: 8224560: (tz) Upgrade time-zone data to
> >> tzdata2019a and 8225580: tzdata2018i integration causes test failures
> >> on jdk-
> >> 13
> >>
> >>
> >>
> >> On 05/07/2019 15:16, Ramanand Patil wrote:
> >>> Hi all,
> >>> Please review the patch for tzdata2019a integration into jdk project.
> >>> Webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.00/
> >>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8224560
> >>> https://bugs.openjdk.java.net/browse/JDK-8225580
> >>>
> >>> Summary:
> >>> - The fix contains cumulative tzdata changes from tzdata2018i and
> >> tzdata2019a, as tzdata2018i was not integrated into jdk/jdk earlier.
> >>> - In JDK-13/14, multiple failures were seen during integration of
> >>> tzdata2018i
> >> (JDK-8225580), those are fixed now. Many thanks to Naoto for
> >> providing a fix for this in CLDRConverter.java.
> >>
> >> I would guess this is due to the CLDR update (JDK-8221432: Upgrade
> >> CLDR to Version 35.1) in OpenJDK 13, making TimeZone.getAvailableIDs
> >> inappropriate in some way?
> >>
> >> Fix looks good. One minor change:
> >>
> >> +AVAILABLE_TZIDS = new
> >> + HashSet(ZoneId.getAvailableZoneIds());
> >>
> >> is missing the  or <>:
> >>
> >> +AVAILABLE_TZIDS = new
> >> + HashSet<>(ZoneId.getAvailableZoneIds());
> >>
> >> Will this fix also resolve JDK-8225580? If so, it's probably worth
> >> mentioning both bug IDs in the commit.
> > Yes, this fix will also resolve JDK-8225580, hence included in the subject
> line.
> > And thank you, I will add both bug IDs in the commit message.
> >>
> >>> - There are 2 type of test failures in TestZoneInfo310.java file,
> >>> which are
> >> solved in this patch by providing workarounds, But a permanent fix
> >> needs to be added in future for the same. Below are the 2 bugs
> >> created to track the development on it:
> >>>   1. https://bugs.openjdk.java.net/browse/JDK-8223388:
> >> TestZoneInfo310.java fails post tzdata2018i integration:
> >>> This failure is seen for the TimeZones which are having zone rules
> >>> defined
> >> till year 2037 or beyond. For now, the failing zones are skipped.
> >>> The supporting test class ZoneInfo.java has maxYear defined
> >> http://hg.openjdk.java.net/jdk/jdk/file/d01b345865d7/test/jdk/sun/uti
> >> l/cale ndar/zi/Zoneinfo.java#l40, changing this max value greater
> >> than the zone rule's last year also fixes the issue, but further
> >> investigation is needed on why this boundary condition is affecting
> >> the test behavior.
> >>
> >> I wonder if 2037 is in someway related to the rollover of 32-bit time
> values?
> > I think, not directly related but how the test and JDK handles these values.
> > In JDK, the transitions beyond 2037 are delegated to SimpleTimeZone, and I
> think the test somehow miscalculates it.
> > http://hg.openjdk.java.net/jdk/jdk/file/5919b273def6/src/java.base/sha
> > re/classes/sun/util/calendar/ZoneInfo.java#l48
> > I think, I should re-visit and see if these test are really needed
> > now. As per the long standing bug JDK-8166983 suggestion was to remove
> > the whole tests from test/sun/util/calendar/zi
> >>
> >>>   2. JDK-8227293: https://bugs.openjdk.java.net/browse/JDK-8227293
> >> TestZoneInfo310.java fails post tzdata2019a integration for Palestine
> >> zone
> >> rules:
> >>> There are many hacks and assumptions in the class
> >>> sun.util.calendar.ZoneInfoFile.java. This issue looks because of the
> >>> code starting from here:
> >>> http://hg.openjdk.java.net/jdk/jdk/file/963924f1c891/src/java.base/s
> >>> ha
> >>> re/classes/sun/util/calendar/ZoneInfoFile.java#l552
> >>> There is an assumption where the transition date is >=24,(line 577
> >>> and 599)
> >> it assumes it is the "last" rule, but it is not last rule in case of
> >> Asia/Gaza and 

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Peter Levart]

On 7/9/19 1:57 AM, Stuart Marks wrote:
2. Create draft CSR. 


Created:

https://bugs.openjdk.java.net/browse/JDK-8227432

Regards, Peter

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Peter Levart]

Hi Stuart,

On 7/9/19 1:57 AM, Stuart Marks wrote:

Hi Peter,

Thanks for picking this up, for filing the bug (JDK-8227368), and 
developing the fix.


For the current release, I think we should go ahead and put in the 
"right" fix, that is, to define serialVersionUID the conventional way 
-- as a compile-time constant -- and then simply to remove the 
suppression of the serial warning. This will change the specification 
(because the change will appear in serialized-form.html), but that's 
OK, we have the ability to change the specification. Note that this 
will require a CSR. (Changing the svuid value without changing the 
specification would probably require a CSR anyway, since it's a 
behavioral change that affects compatibility.) Might as well fix the 
specification now and be done with it, at least for current and future 
releases.


Right, I agree.



The JDK 11 backport is where we should apply the static initializer. 
That's where changing the specification is more difficult. I think we 
should discuss that separately, though.


Thanks for writing the new test and updating the BogusEnumSet test.

Given that this is a fix for an incompatible change, I think this 
should be fixed as soon as possible. I've raised the priority to P3, 
and I've targeted this bug to JDK 13. We're after RDP1, but important 
fixes can still go in during RDP1 [1] until the next phase, RDP2, 
which begins on July 18. [2] Once this goes into JDK 13, it should 
automatically be propagated to JDK 14, with no manual steps necessary.


With the fairly short time frame left in JDK 13 before RDP2, it would 
be best to move as quickly as possible. If you have time to work in 
this immediately, great, but if not, that's ok; please let me know and 
I'll pick up any or all work that you don't have time to do. 
Regardless, I'll help out any way I can, such as reviewing the code 
and the CSR, doing testing, etc.


Next steps:

1. New changeset with constant version of EnumSet.serialVersionUID.


This is already in the webrev.01 changeset. webrev.02 is an attempt to 
sneak the change without being visible in the serialized-form.html.




2. Create draft CSR.


I'll try that right away.

Regards, Peter



Thanks,

s'marks


[1] http://openjdk.java.net/jeps/3

[2] http://openjdk.java.net/projects/jdk/13/




On 7/7/19 8:11 AM, Peter Levart wrote:

On 7/7/19 4:31 PM, Peter Levart wrote:

On 6/29/19 2:00 AM, Stuart Marks wrote:
Daniel Fuchs pointed me to a weird thing they had to do with the 
serialVersionUID in one of the JMX classes:


https://hg.openjdk.java.net/jdk/jdk/file/c59f36ed7b52/src/java.management/share/classes/javax/management/MBeanAttributeInfo.java#l46 



Essentially the svuid for this class initialized in a static block, 
and its value is conditional based on the value of some system 
property. I don't think using a property is necessary for the 
EnumSet case. However, it does point out something interesting, 
which is that if the svuid is not initialized with a compile-time 
constant, and instead via a static block, the value doesn't appear 
in serialized-form.html. 


Hi Stuart,

I just realized that I was reading your last statement wrong. I 
though it was written as:


...if the svuid is not initialized with a compile-time constant, and 
instead via a static block, the value doesn't appear in serialized form.


And not as:

...if the svuid is not initialized with a compile-time constant, and 
instead via a static block, the value doesn't appear in 
serialized-form.html



You were only concerned about the generated javadoc and not about the 
actual "serialized form". Ok, I get it now. I have prepared webrev.02 
to fix this.



Regards, Peter

RE: RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string

[Thejasvi Voniadka]

Hi Roger,

Please find the updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.4/


I have updated both lines 3924 and 3874 of 
.../java/time/format/DateTimeFormatterBuilder.java.

I have reduced the number of tests to just 3, and the number of locales to just 
1 in java/time/format/TestLocalizedOffsetPrinterParser.java. That should 
provide sufficient coverage towards the functionality.



-Original Message-
From: Thejasvi Voniadka 
Sent: Friday, July 05, 2019 9:16 AM
To: Roger Riggs ; core-libs-dev@openjdk.java.net; 
i18n-...@openjdk.java.net
Subject: RE:  RFR: 8154520: java.time: appendLocalizedOffset() should 
return the localized "GMT" string

Hi Roger,

 

Thank you for the review. I am in transit this weekend, but I will fix this and 
republish as soon as I can.

 

 

From: Roger Riggs 
Sent: Wednesday, July 03, 2019 10:46 PM
To: Thejasvi Voniadka ; 
core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should 
return the localized "GMT" string

 

Hi,

Looks ok, but...

.../java/time/format/DateTimeFormatterBuilder.java
3924: needs a space in "if(" -> "if ("

java/time/format/TestLocalizedOffsetPrinterParser.java

I would cut the number of test cases to a minimum; only need to ensure the code 
is correct.
We don't need to be testing the CLDR data; it is just  pass through.
At least cut the number of different locale's used to cut the risk of 
unexpected maintenance.

Thanks, Roger




On 7/3/19 12:10 PM, HYPERLINK 
"mailto:naoto.s...@oracle.com"naoto.s...@oracle.com wrote:

Looks good. 

Naoto 

On 7/3/19 5:32 AM, Thejasvi Voniadka wrote: 



Hi Naoto, 

Thank you for the review. Please find the updated webrev: 

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.3/ 


The TCKOffsetPrinterParser.java has been reverted back to what it was, except 
for the copyright year and the locale addition. I have incorporated your 
comments to TestLocalizedOffsetPrinterParser.java. 



-Original Message- 
From: Naoto Sato 
Sent: Tuesday, July 02, 2019 9:33 PM 
To: Thejasvi Voniadka HYPERLINK 
"mailto:thejasvi.v.vonia...@oracle.com;; 
HYPERLINK 
"mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; 
HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net 
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should 
return the localized "GMT" string 

Hi Thejasvi, 

Here are my comments to the webrev: 

TCKOffsetPrinterParser.java 

- No need to define Locale_US as a static field, nor have it in the test data 
(data_print_localized) then pass it as an argument to the test. 
Specifying Locale.US in line 572, 578, and 586 should suffice. 

TestOffsetPrinterParser.java 

- Copyright year is 2019. 

- It would be nice if some comments that reads something like "This test relies 
on the localized text of "GMT" in the CLDR." 

- Test class (and the description) should include "Localized", as it is testing 
the implementation of localized version of OffsetIdPrinterParser. 

- Line 64-76: I prefer just instantiating them in the test data, not defining a 
static field for each, unless there will be duplicate in the test data. 

- Line 111, 118, 124, 132: I believe the locale parameter is required. 
Make sure that with Objects.requireNonNull(), or fail if it's null. 

Naoto 

On 7/2/19 7:32 AM, Thejasvi Voniadka wrote: 



Hi Naoto, 

Thank you for the review. I have performed the modifications, and here is the 
updated webrev: 

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.2/ 


I have moved the new tests from TCK area. I have also updated the current TCK 
test to explicitly pass Locale.US while calling format. 




-Original Message- 
From: Naoto Sato 
Sent: Monday, July 01, 2019 9:02 PM 
To: Thejasvi Voniadka HYPERLINK 
"mailto:thejasvi.v.vonia...@oracle.com;; 
HYPERLINK 
"mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; 
HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net 
Subject: Re:  RFR: 8154520: java.time: 
appendLocalizedOffset() should return the localized "GMT" string 

Hi Thejasvi, 

Thanks for fixing this. 

Since those new test cases depend on the CLDR localization, which might change 
in other implementations, those test cases should be in jdk/java/time/test 
directory, as "tck" tests should only test the spec. 
Please create a new test case for this in the "test" directory (with @modules 
jdk.localedata directive) similar to the existing TCK one. Also the current 
test in the TCK should enforce that it runs in Locale.US so that the result 
should match "GMT." 

Naoto 

On 6/28/19 5:59 AM, Thejasvi Voniadka wrote: 



Hi, 

Request you to please review this change. 


JBS:    https://bugs.openjdk.java.net/browse/JDK-8154520 


Description:    At present, the 
"DateTimeFormatterBuilder.appendLocalizedOffset()" method formulates the base 
string as "GMT", without accounting for locale-specific transformations. This